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

fix: pin packaging version in dockerfile.processing_image to fix smoke tests #1635

Merged
merged 5 commits into from
Nov 2, 2021

Conversation

Bento007
Copy link
Contributor

@Bento007 Bento007 commented Nov 1, 2021

Reviewers

Functional:

Readability:


This is the original failing smoke test. Here is the error in our smoke test.

ERROR: packaging 21.2 has requirement pyparsing<3,>=2.0.2, but you'll have pyparsing 3.0.4 which is incompatible.

The python library Packaging is failing during install do to conflicting requirements. Packaging's version 21.2 locks down the version of pyparsing to resolve this pypa/packaging#471.

There is also an error installing the requirements for Owlready2==0.34. This is pinned in chanzuckerberg/single-cell-curation

Changes

  • pin packaging version in dockerfile.processing_image to one that does not lock the pyparsing library version to >3.

@Bento007 Bento007 self-assigned this Nov 1, 2021
@Bento007 Bento007 changed the title Fix: install cython in dockerfile.processing_image fix: install cython in dockerfile.processing_image Nov 1, 2021
@Bento007 Bento007 force-pushed the tsmith/smoke-test-fix branch from a284713 to 999cdfb Compare November 1, 2021 23:27
@codecov
Copy link

codecov bot commented Nov 1, 2021

Codecov Report

Merging #1635 (8df8e56) into main (c5a9cb2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1635   +/-   ##
=======================================
  Coverage   94.52%   94.52%           
=======================================
  Files         104      104           
  Lines        6651     6651           
=======================================
  Hits         6287     6287           
  Misses        364      364           
Flag Coverage Δ
backend 94.52% <ø> (ø)
python 94.52% <ø> (ø)
unitTest 94.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 c5a9cb2...8df8e56. Read the comment docs.

@Bento007 Bento007 requested review from ebezzi and seve November 1, 2021 23:38
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

rubber stamping assuming this is a safe change 😆 Thanks so much, @Bento007 !

@maniarathi
Copy link
Contributor

One question and one ask: why not pin the packaging version instead of creating an explicit dependency on a downstream dependency?

And the ask: can you please make sure to write a comment in code as to why it was pinned? We should remove it in the future once all the packages fast forward in compatibility.

@Bento007 Bento007 changed the title fix: install cython in dockerfile.processing_image fix: pin packaging version in dockerfile.processing_image to fix smoke tests Nov 2, 2021
@Bento007 Bento007 merged commit 82ba9fc into main Nov 2, 2021
@Bento007 Bento007 deleted the tsmith/smoke-test-fix branch November 2, 2021 18:00
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