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

remove spacy dependancy in pyproject.toml #913

Closed
wants to merge 4 commits into from

Conversation

madanucd
Copy link
Contributor

@madanucd madanucd commented Nov 26, 2024

Related issues

Summary

This PR addresses dependency issues with monarch-py deployment to PyPI, fixing errors caused by direct dependencies on external URLs for spacy en_core_sci_sm model.

  • Utilized pystow to download and cache the en_core_sci_sm model from an S3 URL.

  • Implemented automatic unpacking and dynamic directory identification to ensure the model is properly extracted and loaded.

Checks

  • All annotation backend tests were checked and successfully passed.
  • Completed testing of the API development environment to ensure proper integration.

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for monarch-app ready!

Name Link
🔨 Latest commit c675d60
🔍 Latest deploy log https://app.netlify.com/sites/monarch-app/deploys/6746244b433cde00087ff87d
😎 Deploy Preview https://deploy-preview-913--monarch-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 77.41935% with 14 lines in your changes missing coverage. Please review.

Project coverage is 71.17%. Comparing base (af5ab7b) to head (c675d60).

Files with missing lines Patch % Lines
...h_py/implementations/spacy/spacy_implementation.py 61.76% 13 Missing ⚠️
backend/tests/integration/test_solr_association.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #913      +/-   ##
==========================================
+ Coverage   71.14%   71.17%   +0.02%     
==========================================
  Files          91       91              
  Lines        3136     3167      +31     
==========================================
+ Hits         2231     2254      +23     
- Misses        905      913       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptgolden
Copy link
Member

ptgolden commented Nov 26, 2024

Hey @madanucd, are there any substantial changes in those last three commits? I think you may not have pulled from main before making the PR, which made all changes after 838436e a little wonky (they revert all commits from the last month or so). If there aren't changes that aren't semantically meaningful for this PR/issue, I'll cherry pick 838436e and update poetry.lock from its HEAD state in main.

@madanucd
Copy link
Contributor Author

Hi @ptgolden

The only updates in this branch are to the following files:

  • backend/src/monarch_py/implementations/spacy/spacy_implementation.py
  • pyproject.toml

The poetry.lock file was updated due to changes in pyproject.toml.

Merging main into issue-809-dependency-update seems to have flagged multiple files as modified, even though only the files listed above were intentionally updated. This discrepancy might create unnecessary confusion. After reverting the aforementioned merge commit, I did not encounter any build failures or conflicts. That said, I ensured my main branch was up to date before creating this branch.

If cherry-picking the relevant commit works better for you, please feel free to proceed. Alternatively, I can create a clean branch and submit a new PR to ensure clarity, deleting this branch and the current PR. Please let me know how.

@madanucd madanucd requested a review from ptgolden November 27, 2024 14:49
@ptgolden
Copy link
Member

ptgolden commented Nov 27, 2024

Strange- the parent commit of 838436e is 793cd1c from 10/21 (the lock file has diverged since then). I am happy to make a new PR cherry picking 838436e and updating poetry.lock from its current state, if that's OK with you.

@madanucd
Copy link
Contributor Author

Strange- the parent commit of 838436e is 793cd1c from 10/21 (the lock file has diverged since then). I am happy to make a new PR cherry picking 838436e and updating poetry.lock from its current state, if that's OK with you.

That's fine with me. Thank you!

@ptgolden
Copy link
Member

New PR in #920 👍

kevinschaper pushed a commit that referenced this pull request Dec 13, 2024
Copied from #913 from @madanucd

### Related issues

- Closes #809 

### Summary

This PR addresses dependency issues with `monarch-py` deployment to
PyPI, fixing errors caused by direct dependencies on external URLs for
spacy `en_core_sci_sm` model.

- Utilized pystow to download and cache the en_core_sci_sm model from an
S3 URL.

- Implemented automatic unpacking and dynamic directory identification
to ensure the model is properly extracted and loaded.

### Checks

- [x] All annotation backend tests were checked and successfully passed.
- [x] Completed testing of the API development environment to ensure
proper integration.

---------

Co-authored-by: madanucd <madan@tislab.org>
Co-authored-by: Patrick Golden <ptgolden@email.unc.com>
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.

Spacy git dependency breaks pypi deploy
2 participants