-
Notifications
You must be signed in to change notification settings - Fork 10
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
github action for integration pipeline #167
Conversation
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.
The jax pins are also rather sus. If you're worried about the chex failure, you need to wait for the next scvi-tools release
.github/workflows/integration-ci.yml
Outdated
@@ -0,0 +1,98 @@ | |||
name: Tutorials CI |
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.
name: Tutorials CI | |
name: Run tutorials |
I'll be more than happy to implement a cleaner fix when possible, but jax is the first thing that broke. Then, restoring the last working version of jax threw an error with numpy, hence the second pin. Happy to hear about any other fix that makes the action work right now. However, I did what had to be done to be able to work on the action itself so that it runs the pipeline. |
Yeah if you look at the CI log again you'll see that scvi-tools is at fault. You can install it from main for now or simply wait for 1-2 weeks where it'll magically fix itself |
Ah ok. I must have missed the scvi-tools bit then. Maybe we open an issue as a sticky note to clean this up in 1-2 weeks? At the moment, I just need a working environment to test the pipeline and demonstrate that it runs in a github action. I agree that on the long run, we want to set up the testing "the right way" with the latest version of all dependencies. |
@kevinrue whatever works for you :) I just wanted to provide a quick perspective of mine |
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.
test python=3.11 @kevinrue and if all works we can safely merge.
we give ourselves a couple of weeks to figure dependencies
@bio-la we're back on track and i think ready to merge this one finally now |
@@ -26,43 +26,43 @@ classifiers = [ | |||
"Programming Language :: R" | |||
] | |||
|
|||
requires-python = ">= 3.9" | |||
requires-python = ">= 3.11" | |||
|
|||
dependencies = [ |
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.
Just a minor comment, but you don't need to specify transitive dependencies -> anndata, jax etc are already included in e.g. scvi-tools. Only if you need to pin them.
Moreover, dependencies such as pytest
are not runtime dependencies and should be moved into a dev section
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're making a lot of good points, but can we open those as issues, as they will otherwise get lost as comments
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.
True! Sorry, Fabiola merged the PR before my comment went out :)
No description provided.