-
Notifications
You must be signed in to change notification settings - Fork 152
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
(fix): gpu ci uses max version from pyproject.toml
#1714
Conversation
ilan-gold
commented
Oct 14, 2024
- Closes #
- Tests added
- Release note added (or unnecessary)
.github/workflows/test-gpu.yml
Outdated
@@ -51,10 +51,17 @@ jobs: | |||
- name: Nvidia SMI sanity check | |||
run: nvidia-smi | |||
|
|||
- name: Extract max Python version from classifiers | |||
run: | | |||
sudo apt-get update && sudo apt-get install -y cargo && cargo install toml-cli |
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.
You could also use this: https://github.com/marketplace/actions/cargo-install
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.
ah was looking for something like this just now. I also thought cargo came on the ubuntu images by default but evidently not install
?
Also, do you think we should be doing this for all of our CI i.e., extracting from pyproject.toml
?
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.
Ok I think that is only for rust projects: https://github.com/scverse/anndata/actions/runs/11330444061/job/31508293550?pr=1714
Hmm no, I think this may be what I saw before actually. Github needs the cargo path without a rust project so I think I'll just go back
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 still think that using that action is better, because it sets up PATH and uses caching.
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.
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 read this as cargo
not being on the path
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1714 +/- ##
==========================================
+ Coverage 84.45% 86.93% +2.48%
==========================================
Files 40 40
Lines 6039 6039
==========================================
+ Hits 5100 5250 +150
+ Misses 939 789 -150 |
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.
Awesome! I’m not a huge fan of most1 short-form CLI parameters, so if you could switch the more obscure ones to long-form I’d even be happier.
Also \K
means something regarding capture right? If I’m right, maybe use a capture group instead, I think people are more likely to know that one. Otherwise please add a comment explaining what it is.
Footnotes
-
except super-well known ones like
something -vv
,grep -P/-E
, ortail -n1/-c5
or so ↩
I aim to please :) |
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.
Ah great, now I understand what’s going on!