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

feat: modernize with PDM #304

Merged
merged 8 commits into from
Jan 8, 2024
Merged

feat: modernize with PDM #304

merged 8 commits into from
Jan 8, 2024

Conversation

frostming
Copy link
Contributor

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

Close #303

wey-gu
wey-gu previously approved these changes Jan 5, 2024
Copy link
Contributor

@wey-gu wey-gu left a comment

Choose a reason for hiding this comment

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

THIS IS CRAZY....

THANKS @frostming 🤦‍♂️

We need a badge in readme later.

@wey-gu
Copy link
Contributor

wey-gu commented Jan 5, 2024

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
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

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

@wey-gu wey-gu requested a review from Nicole00 January 7, 2024 07:47
@wey-gu
Copy link
Contributor

wey-gu commented Jan 8, 2024

@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
Copy link
Contributor

Nicole00 commented Jan 8, 2024

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
Copy link
Contributor

Nicole00 commented Jan 8, 2024

@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
Copy link
Contributor Author

Never mind, I will resolve it.

@frostming
Copy link
Contributor Author

frostming commented Jan 8, 2024

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

Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
@Nicole00
Copy link
Contributor

Nicole00 commented Jan 8, 2024

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
Copy link
Contributor

wey-gu commented Jan 8, 2024

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
Copy link
Contributor Author

frostming commented Jan 8, 2024

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
wey-gu previously approved these changes Jan 8, 2024
@@ -27,35 +27,31 @@ jobs:

Copy link
Contributor

Choose a reason for hiding this comment

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

@Nicole00 we could add 3.6 3.7 3.8 back in separate PR after this being merged. :D

Copy link
Contributor Author

@frostming frostming Jan 8, 2024

Choose a reason for hiding this comment

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

AFAIK the setup-python action can no longer install 3.6 easily. And some of our dev dependencies don't work on 3.6.

dev = [
"black==22.8.0; python_version >= \"3.7\"",
]
test = [
"pytest; python_full_version >= \"3.7.1\"",
"pytest-cov; python_full_version >= \"3.7.1\"",
]
example = [
"prettytable; python_full_version >= \"3.7.1\"",
"pandas; python_full_version >= \"3.7.1\"",
]

That's the trouble part what I mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it, let's put CI with 3.9+ as is, and keep the requirement broader to defer the decision of changing minimal py.

We could make CI best effort in a slightly broader in other PR.

@Nicole00 This seems to be a good enough timing for at least dropping 3.6 :D

@wey-gu
Copy link
Contributor

wey-gu commented Jan 8, 2024

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
Copy link
Contributor

wey-gu commented Jan 8, 2024

/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.

Signed-off-by: Frost Ming <me@frostming.com>
@frostming
Copy link
Contributor Author

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

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

Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
]
test = [
"pytest; python_full_version >= \"3.7.1\"",
"pytest-cov; python_full_version >= \"3.7.1\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we have to give a version for pytest-cov?

ref: nedbat/coveragepy#1376

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded the version

Signed-off-by: Frost Ming <me@frostming.com>
@Nicole00 Nicole00 merged commit b63cd38 into vesoft-inc:master Jan 8, 2024
9 checks passed
@wey-gu
Copy link
Contributor

wey-gu commented Jan 8, 2024

🥳🎉 cc @QingZ11

@frostming frostming deleted the feat/pdm branch January 8, 2024 10:55
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.

modernize nebula-py with PDM
4 participants