Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create Invalidations.yml #920

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Create Invalidations.yml #920

merged 1 commit into from
Sep 5, 2022

Conversation

ranocha
Copy link
Contributor

@ranocha ranocha commented Sep 1, 2022

@codecov-commenter
Copy link

Codecov Report

Merging #920 (7c747cb) into master (91f1241) will decrease coverage by 0.85%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #920      +/-   ##
==========================================
- Coverage   80.16%   79.30%   -0.86%     
==========================================
  Files          36       36              
  Lines        2919     2793     -126     
==========================================
- Hits         2340     2215     -125     
+ Misses        579      578       -1     
Impacted Files Coverage Δ
src/HTTP.jl 72.97% <0.00%> (-13.70%) ⬇️
src/clientlayers/ContentTypeRequest.jl 50.00% <0.00%> (-7.15%) ⬇️
src/clientlayers/CanonicalizeRequest.jl 62.50% <0.00%> (-4.17%) ⬇️
src/access_log.jl 89.83% <0.00%> (-3.51%) ⬇️
src/Exceptions.jl 88.23% <0.00%> (-2.95%) ⬇️
src/parseutils.jl 53.33% <0.00%> (-2.92%) ⬇️
src/Pairs.jl 60.86% <0.00%> (-1.64%) ⬇️
src/sniff.jl 84.21% <0.00%> (-1.41%) ⬇️
src/connectionpools.jl 71.91% <0.00%> (-0.82%) ⬇️
src/ConnectionPool.jl 86.36% <0.00%> (-0.77%) ⬇️
... and 23 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@quinnj quinnj merged commit 4c13891 into JuliaWeb:master Sep 5, 2022
@quinnj
Copy link
Member

quinnj commented Sep 21, 2022

@ranocha, I'm partially regretting merging this. I have a PR open where this new action fails (https://github.com/JuliaWeb/HTTP.jl/actions/runs/3099619353/jobs/5018972872), but the output is......non-existant? Like, I can't find any output telling me why the job failed, or what to do about the failure. Am I missing something obvious in the output there? Can we improve the github action experience to be more useful?

@ranocha
Copy link
Contributor Author

ranocha commented Sep 23, 2022

The step ("report invalidation counts") just before the failing step reports the reason (see https://github.com/JuliaWeb/HTTP.jl/actions/runs/3099619353/jobs/5018972872#step:9:3). The number of invalidations increased be 2 (from 723 to 725), so it does not appear to be a big deal to me at first sight. If you want to dig deeper, you need to work with SnoopCompile.jl locally.
If I were you, I would go ahead and merge this PR - and investigate the invalidations locally, trying to fix the biggest offenders. Just following the docs of SnoopCompile.jl should work fine (it did for me).

@ranocha ranocha deleted the patch-1 branch September 25, 2022 10:57
@quinnj
Copy link
Member

quinnj commented Oct 6, 2022

Yeah, my comment was more directed to the fact that

 echo "Invalidations on default branch: 723 (723 via deps)" >> $GITHUB_STEP_SUMMARY
  echo "This branch: 725 (725 via deps)" >> $GITHUB_STEP_SUMMARY

is, in my opinion, pretty unclear as to what is actually being communicated. What is "default branch"? What is "$GITHUB_STEP_SUMMARY"? What is the 732 vs. (723 via deps) and why would those be different?

Do you see what I mean? The invalidations job failed and I go look at this output and I just find it very unhelpful. Can it not spell out the individual invalidations that changed between default branch vs. PR? Can it organize the information in a table or something that would be more clear to someone passing by and wanting to see and understand quickly what went wrong?

@ranocha
Copy link
Contributor Author

ranocha commented Oct 6, 2022

You're definitely right, there is a lot of potential to improve the action. Some ideas are floating around, e.g., https://discourse.julialang.org/t/potential-performance-regressions-in-julia-1-8-for-special-un-precompiled-type-dispatches-and-how-to-fix-them/86359/22 with an open PR timholy/SnoopCompile.jl#303. I think it will take some time but we will be able to improve it

fredrikekre added a commit to fredrikekre/HTTP.jl that referenced this pull request Nov 24, 2023
This has been failing for quite some time with some internal error. Also
in the working case it is a bit unclear what the actionable thing is
when it errors (xref
JuliaWeb#920 (comment)).
fredrikekre added a commit that referenced this pull request Nov 24, 2023
* Fix tests for new version of `ConcurrentUtilities`

`ConcurrentUtilities` changed an (internal) fieldname of the `Pool` type
from `max` to `limit`. This field is used in HTTP.jl tests so this patch
works around the issue.

* Remove invalidation CI check

This has been failing for quite some time with some internal error. Also
in the working case it is a bit unclear what the actionable thing is
when it errors (xref
#920 (comment)).
nickrobinson251 pushed a commit that referenced this pull request Feb 17, 2024
* Fix tests for new version of `ConcurrentUtilities`

`ConcurrentUtilities` changed an (internal) fieldname of the `Pool` type
from `max` to `limit`. This field is used in HTTP.jl tests so this patch
works around the issue.

* Remove invalidation CI check

This has been failing for quite some time with some internal error. Also
in the working case it is a bit unclear what the actionable thing is
when it errors (xref
#920 (comment)).
nickrobinson251 pushed a commit that referenced this pull request Feb 18, 2024
* Fix tests for new version of `ConcurrentUtilities`

`ConcurrentUtilities` changed an (internal) fieldname of the `Pool` type
from `max` to `limit`. This field is used in HTTP.jl tests so this patch
works around the issue.

* Remove invalidation CI check

This has been failing for quite some time with some internal error. Also
in the working case it is a bit unclear what the actionable thing is
when it errors (xref
#920 (comment)).
nickrobinson251 added a commit that referenced this pull request Feb 18, 2024
* Fix tests for new version of `ConcurrentUtilities`

`ConcurrentUtilities` changed an (internal) fieldname of the `Pool` type
from `max` to `limit`. This field is used in HTTP.jl tests so this patch
works around the issue.

* Remove invalidation CI check

This has been failing for quite some time with some internal error. Also
in the working case it is a bit unclear what the actionable thing is
when it errors (xref
#920 (comment)).

Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants