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

db optional dependency #415

Merged
merged 9 commits into from
Sep 6, 2024
Merged

db optional dependency #415

merged 9 commits into from
Sep 6, 2024

Conversation

Duncan-Hunter
Copy link
Contributor

#412

To install with psycopg2 and sqlmodel, install with either
poetry add nannyml[db]
or
poetry install --all-extras etc (https://python-poetry.org/docs/pyproject/#extras)

Loosens s3fs and gcsfs requirements. Raises import errors if nannyml.io.db is imported directly. The warning/error logic can be changed if needed.

Probably needs a version bump (it's breaking for anyone importing it and not specifying the extra). And probably needs some doc updates. Let me know what you think.

Doesn't work poetry run tox, but neither does the main branch, so :/.

Works with poetry run pytest

Copy link
Collaborator

@michael-nml michael-nml left a comment

Choose a reason for hiding this comment

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

Hey @Duncan-Hunter, thank you for putting this PR together! Looks pretty good!

I just have a concern around generating warnings for everyone using the nannyml package without the db extra installed. Think that's something we'll want to tackle to avoid confusion for users.

nannyml/__init__.py Outdated Show resolved Hide resolved
@nnansters
Copy link
Contributor

Doesn't work poetry run tox, but neither does the main branch, so :/.

I'll take a quick look into this!

@Duncan-Hunter
Copy link
Contributor Author

Duncan-Hunter commented Aug 5, 2024

Doesn't work poetry run tox, but neither does the main branch, so :/.

I'll take a quick look into this!

To be more helpful - this was an error with sqlmodel and the objects in io.db.entities.py being defined/imported multiple times I think.

@nnansters
Copy link
Contributor

Hmm, I recall seeing that message pop up but can't seem to reproduce it either locally or in the automated builds here.
If it passes the builds here I'd give it the stamp of approval. What version of Python are you running locally?

Did a quick experiment with your changes and works as I would expect, very nice. We can apply the same principles for S3FS / GCP / Azure client libraries too upon parsing URLs.

For me this is good to merge!

@Duncan-Hunter
Copy link
Contributor Author

Great, thanks. There's probably some more stuff to add around docs and bumping the version, but perhaps that's better left to you.

For tox, should I raise this in a separate issue?

@Duncan-Hunter
Copy link
Contributor Author

Hi - is there anything I can do to get this merged?

@nnansters
Copy link
Contributor

Yeah, reminding me to do it 😄

Just kidding, this will just need a quick review of the docs for installation. We should have some time to do that this week.

As soon as that is done we can merge and probably also release a new version of the library!

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

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

Project coverage is 74.72%. Comparing base (0750630) to head (522a3bc).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
nannyml/__init__.py 54.54% 5 Missing ⚠️
nannyml/io/__init__.py 37.50% 5 Missing ⚠️
nannyml/io/db/entities.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
- Coverage   77.02%   74.72%   -2.30%     
==========================================
  Files         109      109              
  Lines        9663     9702      +39     
  Branches     1746     1750       +4     
==========================================
- Hits         7443     7250     -193     
- Misses       1722     1965     +243     
+ Partials      498      487      -11     

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

@nnansters nnansters merged commit b614dec into NannyML:main Sep 6, 2024
4 of 5 checks passed
@nnansters
Copy link
Contributor

Merged! Had some annoyance with one of the workflows, but it probably had to do with PyPi "trusted publishers", so to be expected given this was running from your account.

Thanks again for your contribution, very useful and much appreciated!

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