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: docker and dependency overhaul #47

Merged
merged 9 commits into from
May 10, 2022
Merged

maint: docker and dependency overhaul #47

merged 9 commits into from
May 10, 2022

Conversation

eilidhmacnicol
Copy link
Collaborator

@eilidhmacnicol eilidhmacnicol commented May 9, 2022

Changes proposed in this pull request

fixes #46 as well as resolving the dependency on niworkflows/feat/infants-rodents for the EPI reference workflow.

Depends on nipreps/nirodents#57 being merged for a new nirodents tag, then a new release can be made here to update the docker hub image.

Documentation that should be reviewed

@pep8speaks
Copy link

pep8speaks commented May 9, 2022

Hello @eilidhmacnicol! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-10 13:36:29 UTC

@eilidhmacnicol eilidhmacnicol changed the title pin: scikit-image >= 0.19 maint: docker and dependency overhaul May 10, 2022
@eilidhmacnicol eilidhmacnicol force-pushed the pin/skimage branch 2 times, most recently from 092f47b to 3f9234e Compare May 10, 2022 12:34
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Looks good. I think we can save trouble in the long run by just dropping the tests where we call setup.py directly. The officially recommended ways of doing things are to use python -m build to create sdist or wheel, and pip install to install anything.

Apologies for writing these at some point in the past; I thought it was being resilient, but it turns out to have just proved why the entire ecosystem is moving away from these strategies.

@oesteban
Copy link
Member

Totally agreed. Let's migrate asap

@eilidhmacnicol
Copy link
Collaborator Author

eilidhmacnicol commented May 10, 2022

Thanks both for the speedy reviews.

Let's migrate asap

Is this the sort of change that I should wait for before tagging and updating Docker, or should I go ahead to see if it solves #46 ?

@eilidhmacnicol eilidhmacnicol merged commit d153551 into master May 10, 2022
@eilidhmacnicol eilidhmacnicol deleted the pin/skimage branch May 10, 2022 15:07
@oesteban
Copy link
Member

It's not critical, packages seem to build just fine. But it is likely that is not going to be the case for long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (To be released)
Development

Successfully merging this pull request may close these issues.

Singularity: No space left on device
4 participants