vesoft-inc / nebula-python

Client API of Nebula Graph in Python
191 stars 75 forks source link

feat: modernize with PDM #304

Closed frostming closed 7 months ago

frostming commented 8 months ago

Signed-off-by: Frost Ming me@frostming.com

Close #303

wey-gu commented 8 months ago

Don't know what to say, thanks @frostming for modernizing NebulaGraph-python, and welcome to the community!

cc @Nicole00 @QingZ11

I prefer to wait till other two ongoing PR being merged to get this merged :-D.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (fce0b09) 77.83% compared to head (71e6d82) 77.83%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #304 +/- ## ======================================= Coverage 77.83% 77.83% ======================================= Files 18 18 Lines 2423 2423 ======================================= Hits 1886 1886 Misses 537 537 ```

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

wey-gu commented 8 months ago

@Nicole00 I think we could merge this first before the other ongoing PR?

As either way will involve conflict handling, we may merge this ASAP.

If it's agreed we could ask @frostming to rebase for the last time :D

Nicole00 commented 7 months ago

It's really an amazing pr! Thanks @frostming for updating the package manager way for this product, It's a great honor for us to receive your pr.

Nicole00 commented 7 months ago

@Nicole00 I think we could merge this first before the other ongoing PR?

As either way will involve conflict handling, we may merge this ASAP.

If it's agreed we could ask @frostming to rebase for the last time :D

Sorry I already merged the the other pr, and now there's some conflict for this one. @frostming

frostming commented 7 months ago

Never mind, I will resolve it.

frostming commented 7 months ago

One concern, I saw the master has changed the minimum python version to 3.9 in the platform, but didn't specify it in python_requires, which is the standard metadata for this purpose. I just want to confirm are we officially dropping support for Python < 3.9? So I can specify it explicitly as requires_python = ">=3.9", is this change okay for the project?

@Nicole00 @wey-gu

Nicole00 commented 7 months ago

One concern, I saw the master has changed the minimum python version to 3.9 in the platform, but didn't specify it in python_requires, which is the standard metadata for this purpose. I just want to confirm are we officially dropping support for Python < 3.9? So I can specify it explicitly as requires_python = ">=3.9", is this change okay for the project?

@Nicole00 @wey-gu

Confirm with @wey-gu , we can support >=3.6. It will be ok to keep it with requires_python = ">=3.6.

wey-gu commented 7 months ago

Yes, sorry for the confusion, the reason that PR 3.6~~3.8 was dropped was due to the limitation of pip-complier(only a case in dev requirement), now, with the great PDM it's easy to support them all as our production requirements list is quite small.

Please let's keep 3.6+ back if possible with the magic of PDM and Ming.

frostming commented 7 months ago

It depends on the compatibility strategy of your project. From a view at January 2024, I would recommend >=3.8. But either way is fine for me.

The only trouble is it could be hard to find a dependency resolution to work on both 3.6 and 3.12.

wey-gu commented 7 months ago

It depends on the compatibility strategy of your project. From a view at January 2024, I would recommend >=3.8. But either way is fine for me.

Thanks Ming!

We could consider >=3.8 in the next major version(4.x). It was PDM that made it possible to not break existing 3.6/7 users in this next release.

From a view at January 2024, I would recommend >=3.8

Thanks a lot for this insight!

wey-gu commented 7 months ago
/home/runner/.local/share/pdm/venv/lib/python3.8/site-packages/pdm/resolver/providers.py:158: 
PackageWarning: Skipping oldest-supported-numpy@2023.12.21 because it requires Python>=3.7 but the project claims to work with Python>=3.6. Instead, another version of oldest-supported-numpy that supports Python>=3.6 will be used.
If you want to install oldest-supported-numpy@2023.12.21, narrow down the `requires-python` range to include this version. For example, ">=3.7" should work.

ref: https://github.com/vesoft-inc/nebula-python/actions/runs/7442819063/job/20246850015?pr=304

If needed, we could go with >=3.7 even in this PR.

frostming commented 7 months ago

If needed, we could go with >=3.7 even in this PR.

OK, let's see if the CI still fails(likely 😢 )

wey-gu commented 7 months ago

🥳🎉 cc @QingZ11