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

ENH: Stop NeuroDebian dependency in Dockerfile #2578

Merged
merged 6 commits into from
Oct 7, 2021

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Oct 6, 2021

either through conda or direct download

Changes proposed in this pull request

Documentation that should be reviewed

@oesteban oesteban requested review from effigies and mgxd October 6, 2021 18:06
@oesteban oesteban force-pushed the fix/dockerfile-more-anaconda branch 2 times, most recently from 9e54756 to 2d3f4dc Compare October 6, 2021 18:21
either through conda or direct download
@oesteban oesteban force-pushed the fix/dockerfile-more-anaconda branch from 2d3f4dc to 0953038 Compare October 6, 2021 18:33
Dockerfile Outdated
Comment on lines 282 to 283
mkl-service \
mkl \
Copy link
Member

Choose a reason for hiding this comment

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

The point of pinning these was to reduce the scope of rebuilds to inadvertently introduce sources of numeric variation. Has that changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Has that changed?

Not really. We can still pin some specific versions after being able to build - but pinning with conda is not precisely smooth. I'm not against reintroducing pins on all dependencies.

Dockerfile Outdated
# ABI tags can interfere when running on Singularity
RUN strip --remove-section=.note.ABI-tag /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
# RUN strip --remove-section=.note.ABI-tag /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
Copy link
Member

Choose a reason for hiding this comment

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

Before we merge with this removed, let's push a docker/ branch and ask a user who was experiencing the problem (#2534) to verify that it doesn't return.

To my knowledge, affected users include @sameera2004 and @jbwexler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. Alternatively, I can try to locate Qt5 (now installed by conda) and remove the tag anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Well, pushing a docker/ branch will make either task easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oesteban
Copy link
Member Author

oesteban commented Oct 7, 2021

I'll merge now to allow other PRs be executed. The ABI tag stripping has been restored, so it should not be an issue anymore.

@oesteban oesteban merged commit 3d44335 into master Oct 7, 2021
@oesteban oesteban deleted the fix/dockerfile-more-anaconda branch October 7, 2021 08:27
@codecov-commenter
Copy link

Codecov Report

Merging #2578 (418ae0f) into master (a820912) will not change coverage.
The diff coverage is n/a.

❗ Current head 418ae0f differs from pull request most recent head 2a8a195. Consider uploading reports for the commit 2a8a195 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2578   +/-   ##
=======================================
  Coverage   45.33%   45.33%           
=======================================
  Files          41       41           
  Lines        3121     3121           
=======================================
  Hits         1415     1415           
  Misses       1706     1706           
Impacted Files Coverage Δ
fmriprep/cli/workflow.py 3.29% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a820912...2a8a195. Read the comment docs.

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