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

cleanup #9

Merged
merged 1 commit into from Jul 16, 2021
Merged

cleanup #9

merged 1 commit into from Jul 16, 2021

Conversation

yairchn
Copy link
Member

@yairchn yairchn commented Jul 15, 2021

this PR is a running PR to clean the code and validate in buildkite that is keeps passing all tests

@yairchn yairchn force-pushed the yc/cleanup branch 3 times, most recently from 14c901c to 41fafaf Compare July 16, 2021 16:03
clean EDMF_Environment.jl

clean all if S commands

cleanup unused closures

back to 7388e90 unused closures

clean unused function

remove diagnostic updratfs

remove diagnostic updratfs bool

remove bulksteady

clean entr_strcat

clean entr_strcat

clean pres struct

some extra cleanups

remove buoyant_stract

remove buoyant_stract
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@charleskawczynski
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 16, 2021

Build succeeded:

@bors bors bot merged commit 21256be into main Jul 16, 2021
@bors bors bot deleted the yc/cleanup branch July 16, 2021 16:38
bors bot added a commit that referenced this pull request Mar 6, 2022
876: Make artifacts download lazily r=charleskawczynski a=charleskawczynski

A long time ago, we decided to add a flag in ArtifactWrappers, `local_run`, which allows users to make a temporary download folder to avoid race conditions (downloading Artifacts is not safe to race conditions). The problem is that this basically completely side-steps the utility of lazily downloading the artifacts on remote machines. So, right now, users leverage the utility of Artifacts because the `local_run` flag we have set to `isempty(get(ENV, "CI", ""))`, which is true on buildkite and GHA CI, but all of CI infra is downloading these artifacts on every build.

This PR changes this by doing a proper lazy download during the initial `steps` in the buildkite pipeline (which happens in serial, but avoids the race condition). The advantage of this is that the very first time we do this, we'll pay the cost, but subsequent PRs should now be doing a lazy download.

We should probably just remove this flag in ArtifactWrappers, since it kind of defeats the purpose of Artifacts to begin with, the logic of always downloading can be done by users.

With this PR, we should basically no longer run into these failures:

```
ERROR: LoadError: HTTP/2 302 (Operation too slow. Less than 1 bytes/sec transferred the last 20 seconds) while requesting https://caltech.box.com/shared/static/7upt639siyc2umon8gs6qsjiqavof5cq.nc
--
  | Stacktrace:
  | [1] (::Downloads.var"#9#18"{IOStream, Base.DevNull, Nothing, Vector{Pair{String, String}}, Float64, Nothing, Bool, Bool, String, Int64, Bool, Bool})(easy::Downloads.Curl.Easy)
  | @ Downloads /central/software/julia/1.7.0/share/julia/stdlib/v1.7/Downloads/src/Downloads.jl:369
  | [2] with_handle(f::Downloads.var"#9#18"{IOStream, Base.DevNull, Nothing, Vector{Pair{String, String}}, Float64, Nothing, Bool, Bool, String, I
```

Also, we should probably apply this fix to other repos that have used this pattern.

Co-authored-by: Charles Kawczynski <kawczynski.charles@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.

None yet

2 participants