Conversation
f5d9021 to
3dba1e2
Compare
3dba1e2 to
f5cf820
Compare
tom-osika
left a comment
There was a problem hiding this comment.
It looks like the caching is, on average, slowing down the ubuntu poetry build. For example, compare the latest run against an earlier run without caching. It seems like this is mainly due to the overhead of initializing setup-python. The cache is about 95 MB for ubuntu + poetry, but around 60% of that for poetry and pip on macos. Happy to remove the caching or just leave it for now.
@tom-osika made 2 comments.
Reviewable status: 1 unresolved discussion, platform LGTM missing.
.github/workflows/pip.yml line 48 at r1 (raw file):
run: .github/ci_build_test shell: zsh -efuo pipefail {0} ubuntu_noble_pip:
Note that I ran into the following issue when trying to enable pip caching on ubuntu: actions/setup-python#719
One suggestion was to try force clearing the python cache. I don't know the full effects of removing the python cache each time, and this feels like it may be overkill for our CI, but I can try it if desired.
tom-osika
left a comment
There was a problem hiding this comment.
@tom-osika made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on jwnimmer-tri).
a discussion (no related file):
+assignee:@jwnimmer-tri
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 2 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on tom-osika).
a discussion (no related file):
Ah. I see better now what you were getting at.
For some reason, I thought that the version identifier drake~=1.50 was shorthand for drake >= 1.50.0; drake < 1.51.0, but it's
actually shorthand for drake >= 1.50.0; drake < 2.0.0.
However, per the comments below, I agree that the GHA pip caching is not a big problem in practice, because of how DEE has Renovate hooked up to bump the minimum (and therefore rebuild the cache) upon every Drake release.
Still, since DEE is supposed to be an example of how we recommend that downstream users set up their project, turning on the caching lays a trap in case they copy our GHA yaml but don't do the work of setting up Renovate. I believe in that case, their GHA would not automatically test against the newer versions of Drake?
Given that possible trap, it seems to me like we should probably err on the side of caution and not enable caching, closing the issue as "won't fix". WDYT?
drake_pip/.github/workflows/ci.yml line 43 at r1 (raw file):
# setup-python will use the version of drake that is in the cache, # until the requirements.txt file is updated # (see the cache-dependency-path argument below).
nit This particular part of the comment seems spurious.
Code quote:
# (see the cache-dependency-path argument below).
tom-osika
left a comment
There was a problem hiding this comment.
@tom-osika made 1 comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on jwnimmer-tri).
a discussion (no related file):
I believe in that case, their GHA would not automatically test against the newer versions of Drake?
Correct. For example, if someone copies our GHA yaml today, then 1.51 is released in two weeks, their CI would pull in 1.50, not 1.51. It could be a bit misleading based on what's in our pyproject.yaml and requirements.txt.
IMO there are three options:
- Close as won't fix.
- Make this limitation abundantly clear in the yaml or somewhere else in the documentation. Of course this could be easily missed.
- Change the
~=to==. This at least prevents downstream users from being mislead about which version of drake is being pulled in, but doesn't serve as the best example for how to actually pull in drake in practice.
As-is, the caching is sort of a mixed benefit to the repo anyways. The macos builds are definitely faster, but we can't really cache on ubuntu with pip, and the ubuntu poetry examples are actually slower with caching. All of this to say I think closing as won't fix is fine.
This change is