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

github action for integration pipeline #167

Merged
merged 27 commits into from
Feb 23, 2024
Merged

github action for integration pipeline #167

merged 27 commits into from
Feb 23, 2024

Conversation

kevinrue
Copy link
Collaborator

@kevinrue kevinrue commented Feb 8, 2024

No description provided.

Copy link
Collaborator

@Zethson Zethson left a 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

pyproject.toml Outdated Show resolved Hide resolved
.github/workflows/integration-ci.yml Outdated Show resolved Hide resolved
.github/workflows/integration-ci.yml Show resolved Hide resolved
@@ -0,0 +1,98 @@
name: Tutorials CI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: Tutorials CI
name: Run tutorials

@kevinrue
Copy link
Collaborator Author

kevinrue commented Feb 9, 2024

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

I'll be more than happy to implement a cleaner fix when possible, but jax is the first thing that broke.
https://github.com/DendrouLab/panpipes/actions/runs/7831999626/job/21369644762#step:7:52

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.

@Zethson
Copy link
Collaborator

Zethson commented Feb 9, 2024

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

@kevinrue
Copy link
Collaborator Author

kevinrue commented Feb 9, 2024

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.

@Zethson
Copy link
Collaborator

Zethson commented Feb 9, 2024

@kevinrue whatever works for you :) I just wanted to provide a quick perspective of mine

Copy link
Collaborator

@bio-la bio-la left a 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

.github/workflows/integration-ci.yml Outdated Show resolved Hide resolved
@kevinrue
Copy link
Collaborator Author

@bio-la we're back on track and i think ready to merge this one finally now

@kevinrue kevinrue requested a review from bio-la February 20, 2024 20:32
@bio-la bio-la merged commit 469839a into main Feb 23, 2024
3 checks passed
@@ -26,43 +26,43 @@ classifiers = [
"Programming Language :: R"
]

requires-python = ">= 3.9"
requires-python = ">= 3.11"

dependencies = [
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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 :)

@bio-la bio-la mentioned this pull request Feb 23, 2024
@kevinrue kevinrue deleted the kra-gha-integration branch February 23, 2024 10:30
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.

3 participants