-
Notifications
You must be signed in to change notification settings - Fork 7.1k
cache dependencies in prototype tests on a daily basis #5929
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
base: main
Are you sure you want to change the base?
Conversation
By caching the environment, we now get these results:
Now we get a significant relative decrease, but in relation to the total runtime of the workflow, it is still not much 🤷 Given that we can write the cache key generation in Python, IMO it is simple enough to keep. No strong opinion though. |
@@ -1,3303 +0,0 @@ | |||
version: 2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will re-instate this file when the PR is otherwise good to go. With this deleted, I can debug GitHub Actions CI without triggering a full CircleCI build with every commit.
|
||
today = datetime.datetime.utcnow() | ||
yesterday = today - datetime.timedelta(1) | ||
cache_date = f"{today if today.hour > 10 else yesterday:%Y%m%d}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sure we cache the current nightly builds correctly. We will have a "constant" env every day starting from UTC+0 11a:00.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, torchdata
only starts their nightly workflows at UTC+0 11:00. Thus, we should only cache after they are done:
cache_date = f"{today if today.hour > 10 else yesterday:%Y%m%d}" | |
cache_date = f"{today if today.hour >= 12 else yesterday:%Y%m%d}" |
- name: Run prototype tests | ||
shell: bash | ||
run: pytest --durations=20 test/test_prototype_*.py | ||
# - name: Run prototype tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll reinstate this if the PR is otherwise good to go.
No strong opinions on my side. Tiny time savings and small number of lines introduced. @NicolasHug do you have opinion? |
@pmeier following your commits and CI times w/o cache2e27808 -> GHA: https://github.com/pytorch/vision/runs/6272567930 w/ cache I see a bit different times: Without cache:Ubuntu: 8m 1s (https://github.com/pytorch/vision/runs/6272567930)
Windows: 8m 28s (https://github.com/pytorch/vision/runs/6272567996?check_suite_focus=true)
MacOSX: 9m 49s (https://github.com/pytorch/vision/runs/6272568056?check_suite_focus=true)
With cache:Ubuntu: 4m 24s (https://github.com/pytorch/vision/runs/6272688394?check_suite_focus=true)
Windows: 4m 6s (https://github.com/pytorch/vision/runs/6272688479?check_suite_focus=true)
MacOSX: 8m 0s (https://github.com/pytorch/vision/runs/6272688565?check_suite_focus=true)
Am I misinterpreting something ? |
|
My 2 cent is that even the tiniest change can backfire and cause problems. CI and dependencies are very touchy areas, so in general I prefer not to change anything unless it's really worth it. Here there are some gains but as you noted they're not incredibly high, and the prototype test job is far from being a bottleneck on our CI anyway. |
Addresses #5914 (comment). Timings:
There are some gains, but they are quite insignificant compared to the overall runtime of the workflow. This is due to 2 factors:
I'll try to cache the environment instead and report the timings again.