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

Migrate to pyproject.toml and hatchling #6

Merged
merged 16 commits into from
Aug 21, 2023
Merged

Migrate to pyproject.toml and hatchling #6

merged 16 commits into from
Aug 21, 2023

Conversation

JoshMock
Copy link
Member

@JoshMock JoshMock commented Aug 11, 2023

I used Hatch to convert setup.py into pyproject.toml, and then did some cleanup to drop dev-requirements.txt, ensure nox scripts keep running, etc.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks, moving from setup.py to pyproject.toml is great! Since the CI setups of elasticsearch-py and elasticsearch-serverless-are based on what urllib3 was doing in 2021, I have opinions. 😬

The main thing I'm wondering about is inlining scripts inside pyproject.toml. While setup.py is bad because it requires executing arbitrary Python code and building a wheel to learn about simple facts such as the package name, I don't think everything should move to pyproject.toml:

  • In particular, I think keeping scripts in a Python file where they can be highlighted, linted, formatted and expanded easily is a better option. (And I like using scripts, I find using nox much simpler than tox, which planned at some point to support scripts too, but apparently that never happened.)
  • Additionally, moving to Hatch the project manager is harder to undo that just moving to Hatchling the build backend. I prefer using small tools that can do one thing well.

But then I am biased as I've been using nox for a long time now. And apparently Hatch is so easy to use that this is the second time I'm seeing a coworker moving to it :) elastic/rally-tracks#289

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@JoshMock
Copy link
Member Author

@pquentin Good points! And all things I was hoping to uncover by opening a draft PR.

I'm more than happy to keep things in Nox. I really like how Hatch manages envs and their dependencies, but it's more of a convenience than a necessity.

Part of the inspiration here was that elasticsearch-py has an open issue where Seth seconded the use of Hatch to help migrate away from setup.py. But your point about just using Hatchling for packaging strikes a nice balance. I'll see about shifting back that direction.

@JoshMock
Copy link
Member Author

@pquentin cleaned things up a bit. what do you think?

@JoshMock
Copy link
Member Author

I kept Hatch as a dependency for version bumping and build targets, but I assume I could drop tool.hatch.version and tool.hatch.build.targets.sdist from the config if you only wanted to use hatchling.

Not needed if we're not aligning versions with the Elastic stack for
serverless. See elastic/devtools-team#618
@JoshMock
Copy link
Member Author

Actually I just talked myself out of tool.hatch.version. We won't really need to automate versioning of this package since it will not need to stay in sync with the ES stack version.

@JoshMock JoshMock marked this pull request as ready for review August 16, 2023 19:49
@JoshMock JoshMock changed the title Migrate to pyproject.toml and Hatch Migrate to pyproject.toml and hatchling Aug 16, 2023
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! Is there any way to test the CI parts?

.ci/make.sh Show resolved Hide resolved
.readthedocs.yml Outdated Show resolved Hide resolved
.readthedocs.yml Outdated
Comment on lines 10 to 12
jobs:
pre_install:
- "python -m pip install sphinx-rtd-theme sphinx-autodoc-typehints"
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed since those dependencies are now in docs extra requirements?

Copy link
Member Author

Choose a reason for hiding this comment

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

nox -s docs was failing due to missing config values that sphinx-rtd-theme needed. I just looked into it more closely and needed to enforce versions on sphinx and the theme to work around it. Fixed in 16c972f.

Copy link
Member

Choose a reason for hiding this comment

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

I wondered what was going and investigated too to be able to tell when we can drop the pins. As you've noticed, the root cause is that the two sphinx packages that we install currently require incompatible versions of Sphinx:

Then pip considers that the plugin "at fault" is sphinx-rtd-theme. I'm not sure how the resolver works, maybe it's the only package that does not support Sphinx 7. Anyway, pip then tries to find a version that does in fact support Sphinx 7:

INFO: pip is looking at multiple versions of sphinx-rtd-theme to determine which version is compatible with other requirements. This could take a while.
Collecting sphinx-rtd-theme (from elasticsearch-serverless==0.1.0)
  Obtaining dependency information for sphinx-rtd-theme from https://files.pythonhosted.org/packages/c2/73/946cdd32f29c87843edaa059b9fc52ab9267d1137b3a76dee1d5
9d02a01a/sphinx_rtd_theme-1.2.1-py2.py3-none-any.whl.metadata                  
  Using cached sphinx_rtd_theme-1.2.1-py2.py3-none-any.whl.metadata (4.5 kB)
  Using cached sphinx_rtd_theme-1.2.0-py2.py3-none-any.whl (2.8 MB)
  Using cached sphinx_rtd_theme-1.1.1-py2.py3-none-any.whl (2.8 MB)
  Using cached sphinx_rtd_theme-1.1.0-py2.py3-none-any.whl (2.8 MB)
  Using cached sphinx_rtd_theme-1.0.0-py2.py3-none-any.whl (2.8 MB)
  Using cached sphinx_rtd_theme-0.5.2-py2.py3-none-any.whl (9.1 MB)
  Using cached sphinx_rtd_theme-0.5.1-py2.py3-none-any.whl (2.8 MB)

Since sphinx-rtd-theme 0.5.1 is the latest release without bounds, pip considers that it does support Sphinx 7. And then installs it. This is something we are likely to continue seeing in the future, given:

I can see two ways out of this issue:

  1. We can use a different theme like which updates faster than sphinx-autodoc-typehints (but does not use a lower bound): https://github.com/pradyunsg/furo.

  2. We can pin Sphinx ourselves, and periodically bump to the latest version that sphinx-rtd-theme supports:

    docs = [                                                                                                                                                       
        # sphinx-rtd-theme does not support Sphinx 7 yet                                                                                                           
        "sphinx<7",                                                                                                                                                
        "sphinx-rtd-theme",                                                                                                                                        
        "sphinx-autodoc-typehints",
    ]

I think I prefer the second approach. Although it does not have to block the pull request, we can do this later as this pull request is otherwise ready to be merged IMO.

MANIFEST.in Outdated Show resolved Hide resolved
docs/sphinx/conf.py Outdated Show resolved Hide resolved
docs/sphinx/index.rst Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@JoshMock JoshMock requested a review from pquentin August 17, 2023 16:06
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

This looks great now! Thanks for iterating.

@JoshMock
Copy link
Member Author

Of course! Thanks for all the useful feedback @pquentin. First time migrating from a setup.py. 😄

@JoshMock JoshMock merged commit 6f13e85 into main Aug 21, 2023
14 checks passed
@JoshMock JoshMock deleted the hatch branch August 21, 2023 18:15
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.

2 participants