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

MAINT: Uniformise how to handle dependency install cell #318

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Aug 27, 2024

I opted to keep using the requirement files here, as opposed to list the packages in the pip install line as we already assume the users work from a cloned/full repo version for these notebooks (as opposed to the IRSA-tutorials that are expected to be used as self contained standalone notebooks).

Also, for easier book-keeping, I added one requirements file for each of the notebooks. They can still refer one another (see the one for scale_up), but this would make it easier in the CI and image generation to pick and choose dependency installs as needed.

My editor did a lot of whitespace cleanups, there is a Hide whitespace in the github diff to make it a cleaner review.

Copy link
Contributor

@troyraen troyraen left a comment

Choose a reason for hiding this comment

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

I reviewed the changes in #321 since those changes were done on top of these.
I think these specific changes are working fine, though I found several requirements that I needed to add.

@bsipocz
Copy link
Member Author

bsipocz commented Aug 27, 2024

I think these specific changes are working fine, though I found several requirements that I needed to add.

I would say to go ahead with this then, I can then rebase the other PRs and they will become much cleaner in their diffs.

I also opened nasa-fornax/fornax-documentation#43 that super briefly summarizes that the approach is to have a separate requirements file for each and every notebook, so they can be run separately if needed. From your review I see that I missed one undeclared scikit-image, but the other things are recovered.

@bsipocz bsipocz merged commit d406fe7 into nasa-fornax:main Aug 28, 2024
3 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 28, 2024
MAINT: Uniformise how to handle dependency install cell d406fe7
@bsipocz bsipocz deleted the MAINT_requirements branch August 28, 2024 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants