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

Switch to hatch for build and (dynamic) versioning #345

Merged
merged 11 commits into from
Apr 12, 2024
Merged

Switch to hatch for build and (dynamic) versioning #345

merged 11 commits into from
Apr 12, 2024

Conversation

dhdaines
Copy link
Collaborator

@dhdaines dhdaines commented Mar 22, 2024

You don't actually need to directly use hatch for anything but this gives the option to use it for environment management. Also use its version of setuptools_scm to do dynamic versioning based on Git tags. Also use its plugin for pip-compile to generate a production requirements.txt for Heroku.

So, if you want, you can do:

hatch run dev:check  # runs mypy
hatch run test:test  # runs tests in all python versions
hatch --env dev shell  # gives you a shell with an isolated development environment

But you can also just:

pip install -e .[dev,test]
python run_tests.py all
# etc, etc

Copy link
Contributor

github-actions bot commented Mar 22, 2024

CLI load time: 0:00.05
PR head 4e23d76b630c54fc8df3a3f4c33efafed7f407f7
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.59%. Comparing base (b772bd6) to head (4e23d76).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
- Coverage   93.59%   93.59%   -0.01%     
==========================================
  Files          18       17       -1     
  Lines        2342     2341       -1     
  Branches      520      520              
==========================================
- Hits         2192     2191       -1     
  Misses         85       85              
  Partials       65       65              

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

Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

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

Hum, there's a lot of stuff I don't get about this PR. We should discuss this one live.

.github/workflows/pythonpublish.yml Outdated Show resolved Hide resolved
.github/workflows/pythonpublish.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@dhdaines
Copy link
Collaborator Author

dhdaines commented Mar 25, 2024

Note that to run tests in all of the supported Pythons, you can:

hatch run test:test

Or a single specific version:

hatch run test.py3.7:test

@dhdaines dhdaines changed the base branch from main to dev.mapping-fixing March 25, 2024 19:21
@dhdaines dhdaines force-pushed the dev.hatch branch 2 times, most recently from 1b7bcd7 to 205b2b7 Compare March 25, 2024 19:23
@dhdaines dhdaines changed the title Minimally switch to hatch for build and (dynamic) versioning Switch to hatch for build and (dynamic) versioning Mar 26, 2024
Base automatically changed from dev.mapping-fixing to main March 26, 2024 17:31
@dhdaines dhdaines force-pushed the dev.hatch branch 5 times, most recently from d6a4510 to 070e4d1 Compare March 26, 2024 20:38
@dhdaines dhdaines requested a review from joanise March 26, 2024 20:43
@joanise
Copy link
Collaborator

joanise commented Mar 28, 2024

Wow, I just did some digging, and we have never actually directly used requests or dnspython in this repo, and it's been spurious all along! We could have let eventlet declare its own dependency and leave it at that.

The requests dependency was due to a newbie mistake by me: I added import requests in test_cli.py in 2019 but didn't even use it, yet didn't notice that and added it to requirements.txt.

I think dnspython was added to requirements.txt to lock versions, that one did have a valid justification at the time. But it was still always indirect, never a direct dependency.

It's good to clean this stuff up sometimes! Makes me wonder what other dependency declarations we could safely remove... I suppose hatch test doing matrix testing locally helps validate this kind of question, doesn't it?

pyproject.toml Outdated Show resolved Hide resolved
@joanise
Copy link
Collaborator

joanise commented Mar 28, 2024

Not fully reviewed yet, btw, but I've started playing with this. So far I like what I see. I like how the different dependency group declarations are now chained and nicely DRY.

@joanise
Copy link
Collaborator

joanise commented Mar 28, 2024

The drop in coverage shown at https://app.codecov.io/gh/roedoejet/g2p/pull/345/commits tells me we need .coveragerc, or maybe just our CI workflow needs it as currently written? We didn't use to calculate coverage on g2p/tests, now we do again with 0347281.

@dhdaines
Copy link
Collaborator Author

The drop in coverage shown at https://app.codecov.io/gh/roedoejet/g2p/pull/345/commits tells me we need .coveragerc, or maybe just our CI workflow needs it as currently written? We didn't use to calculate coverage on g2p/tests, now we do again with 0347281.

Ah, it seems like it was just misconfigured. Should be okay now.

@dhdaines
Copy link
Collaborator Author

Just dropping a reference to juftin/hatch-pip-compile#78 here to explain why g2p[prod] cannot just depend on g2p[api]...

@dhdaines
Copy link
Collaborator Author

Not fully reviewed yet, btw, but I've started playing with this. So far I like what I see. I like how the different dependency group declarations are now chained and nicely DRY.

As you can see there are a couple of bugs (though we are able to work around them) in hatch related to this.

Poetry apparently has more robust, but quite non-standard, support for dependency groups - I do still prefer hatch because of its better environment management and interoperability with other tools (like plain old pip for instance).

Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

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

Sending the comments I wrote but once again I cannot finish my review today, sorry.

.github/workflows/pythonpublish.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Copy link
Owner

@roedoejet roedoejet 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 a lot of work, thanks @dhdaines. The process for installing things on my mac was pretty easy, and I agree it's a lot nicer to maintain interoperability with pip (instead of with poetry). I'm leaving this as a comment for now until we sort out the issue @joanise raised on slack.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
.gitignore Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/installation.md Show resolved Hide resolved
g2p/api.py Outdated Show resolved Hide resolved
g2p/mappings/create_ipa_mapping.py Outdated Show resolved Hide resolved
g2p/tests/test_utils.py Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

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

OK, finally done my review, and I really like it. I was able to run everything as documented (well, not the hatch-driven matrix test yet, but the rest). With eventlet 0.36.1, tests run and pass. I would declare that dependency as >=0.36.1 now, except for the fact we're about to remove that dependency anyway. So in the mean time I don't care too much.

I'm really looking forward to merging this, I think once AP agree we can go ahead and merge.

README.md Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

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

In fact, I'm ready to approve this, and we can patch things as needed later.

@dhdaines dhdaines merged commit 6333947 into main Apr 12, 2024
5 of 6 checks passed
@dhdaines dhdaines deleted the dev.hatch branch April 12, 2024 23:33
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.

None yet

3 participants