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(pathy): don't force input paths to Path when de/serializing objects #465

Merged

Conversation

justindujardin
Copy link
Contributor

@justindujardin justindujardin commented Feb 1, 2021

With spaCy 3, saving models to Pathy paths has stopped working.

It turns out that thinc is forcing the input paths to be a Path class by wrapping them "just to be sure". It's friendly and concise, but it messes with inputs that are already path-like objects.

I added tests to demonstrate the failure, and as a fix, I only coerce the type to Path if it is a string.

Related to: justindujardin/pathy#44

 - With the spacy 3 release, I noticed Pathy can't save models because thinc is forcing them to be PosixPath even if they're Pathy on the way in.
 - To fix, only force to Path() if the input is a string
@justindujardin justindujardin changed the title fix(pathy): don't force input paths to PosixPath fix(pathy): don't force input paths to Path when de/serializing objects Feb 1, 2021
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #465 (a74c947) into master (ca42188) will decrease coverage by 0.29%.
The diff coverage is 51.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #465      +/-   ##
==========================================
- Coverage   59.66%   59.37%   -0.30%     
==========================================
  Files         100      101       +1     
  Lines        7079     7148      +69     
==========================================
+ Hits         4224     4244      +20     
- Misses       2855     2904      +49     
Impacted Files Coverage Δ
thinc/layers/logistic.py 100.00% <ø> (ø)
thinc/layers/reduce_first.py 100.00% <ø> (ø)
thinc/layers/reduce_last.py 100.00% <ø> (ø)
thinc/shims/shim.py 100.00% <ø> (ø)
thinc/tests/backends/test_ops.py 0.00% <0.00%> (ø)
thinc/tests/layers/test_layers_api.py 0.00% <ø> (ø)
thinc/tests/layers/test_shim.py 0.00% <0.00%> (ø)
thinc/tests/layers/test_with_transforms.py 0.00% <0.00%> (ø)
thinc/tests/model/test_model.py 0.00% <0.00%> (ø)
thinc/model.py 94.03% <66.66%> (ø)
... and 17 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 3d5ab74...a74c947. Read the comment docs.

@justindujardin justindujardin force-pushed the fix/pathy-serialize-config branch from 9b904e8 to 1be4a17 Compare February 2, 2021 00:23
justindujardin and others added 11 commits February 1, 2021 17:00
Because tensorflow requires `numpy~=1.19.2`, the isolated sdist build
ends up incompatible with the venv after tensorflow is installed, so
instead, install tensorflow and requirements separately and then build
without build isolation.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Note: `pip install -c constraints.txt` doesn't pass the constraints into the nested `pip install` command used to set up the isolated build env, but setting the supposedly equivalent environment variable `PIP_CONSTRAINT` does work because it's still set when the nested pip command runs.
@adrianeboyd adrianeboyd merged commit 816dbb1 into explosion:master Feb 9, 2021
@justindujardin justindujardin deleted the fix/pathy-serialize-config branch February 9, 2021 21:35
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.

None yet

2 participants