NannyML / nannyml

nannyml: post-deployment data science in python
https://www.nannyml.com/
Apache License 2.0
1.97k stars 139 forks source link

db optional dependency #415

Closed Duncan-Hunter closed 2 months ago

Duncan-Hunter commented 3 months ago

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

nnansters commented 3 months ago

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

I'll take a quick look into this!

Duncan-Hunter commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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

nnansters commented 2 months ago

Yeah, reminding me to do it :smile:

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!

codecov[bot] commented 2 months ago

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 :warning:
nannyml/io/__init__.py 37.50% 5 Missing :warning:
nannyml/io/db/entities.py 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nnansters commented 2 months ago

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!