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

Use standard pyproject.toml #57

Merged
merged 21 commits into from
Oct 15, 2024
Merged

Use standard pyproject.toml #57

merged 21 commits into from
Oct 15, 2024

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Oct 7, 2024

Fixes #56.

@hofaflo
Copy link
Contributor

hofaflo commented Oct 10, 2024

Thanks for the PR! Are you already working on getting the checks to pass or should I look into it?

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

Thanks @hofaflo. I tried to modify test.yml, but I'm not sure what you are doing with NumPy. Do you want to install the specific versions depending on the Python version? If so, I'm not sure why it's not working (${{matrix.numpy}} yields --pre numpy, but I thought it should be e.g. numpy==1.22.0)...

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

So what I'm saying is that I don't know how to specify using a pre-release NumPy...

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

I guess this will require using the uv pip interface, which will probably make the process more involved.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

@hofaflo a test is failing because of a user warning. Did I miss something? I don't think this is related to the changes in this PR.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

Regarding building the docs, I'm not sure how you would like to declare edfio as a dependency, any suggestions?

@hofaflo
Copy link
Contributor

hofaflo commented Oct 11, 2024

I'm not sure what you are doing with NumPy. Do you want to install the specific versions depending on the Python version?

We're testing with the oldest and newest supported NumPy version for each Python version. If I understand correctly, for uv we'd have to replace numpy: ["--pre numpy"] with numpy: ["numpy --prerelease=allow"]. However, this flag was introduced to ensure compatibility with NumPy v2.0 before there was a non-pre-release, so we can just do numpy: ["numpy"] now.

a test is failing because of a user warning. Did I miss something? I don't think this is related to the changes in this PR.

This is probably because pytest.warns re-emits unmatched warnings since v8.0.0, which was not used so far as we pinned the dev dependencies to exact versions (something I'd like to keep doing).

Regarding building the docs, I'm not sure how you would like to declare edfio as a dependency, any suggestions?

Simplifying this example from the RTD docs a bit, maybe something like

  commands:
    - pip install uv
    - uv run --extra dev python -m sphinx -T -b html -d docs/_build/doctrees -D language=en docs/source $READTHEDOCS_OUTPUT/html

.github/workflows/test.yml Outdated Show resolved Hide resolved
@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

Hmmm, not sure why the test are not being triggered now?

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

Never mind, I found it.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 11, 2024

Do you want to bump any dev packages while I'm at it @hofaflo?

@hofaflo
Copy link
Contributor

hofaflo commented Oct 14, 2024

Do you want to bump any dev packages while I'm at it?

No thanks, I think that would fit better in a separate PR.

Could you update the section "Development environment" accordingly? E.g. to something like this:

We suggest to use [`uv`](https://docs.astral.sh/uv/getting-started/installation/) for managing project dependencies, but any other standard-compliant tool will work too.

Create a virtual environment with Python 3.9 and install all dependencies:

    uv sync --python=3.9 --all-extras

**When using `uv`, prepend the commands below with `uv run` to make sure they are executed in the virtual environment!**

We'll need to replace poetry in the release pipeline as well. I just added this repo as a trusted publisher on PyPI, so the following setup should work (in release.yml):

  publish:
    needs: test
    runs-on: ubuntu-latest
    permissions:
      id-token: write
    steps:
      - uses: actions/checkout@v4
        with:
          lfs: true
      - name: Setup uv
        uses: astral-sh/setup-uv@v3
      - name: Publish to PyPI
        run: |
          uv build
          uv publish

Finally, we should remove remaining mentions of poetry in README.md and CONTRIBUTING.md

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 14, 2024

Great, will do!

Finally, do you think we can skip

      - name: Install Python ${{ matrix.python-version }}
        run: uv python install ${{ matrix.python-version }}

and instead use

      - name: Pre-commit
        run: |
          uv run --extra dev --python ${{ matrix.python-version }} --with ${{ matrix.numpy }} pre-commit run --all-files
      - name: Pytest
        run: |
          uv run --extra dev --python ${{ matrix.python-version }} --with ${{ matrix.numpy }} pytest -Werror --cov --cov-report term-missing --cov-fail-under=100

?

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 14, 2024

And another question/heads up regarding the dynamic version. If I install the current HEAD, the version shown is 0.4.0.dev94+g47f9643. Is this OK? I'm not sure, because the latest release/tag is v0.4.4, so maybe the dev version should be 0.5.0.dev????????

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 14, 2024

PS: I'm not even sure what's going on, because uv tool run hatch version outputs 0.5.0.dev20+g47f9643, so maybe everything is fine?

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 14, 2024

The dynamic version is incorrect for editable installs unless the project is rebuilt (https://github.com/ofek/hatch-vcs?tab=readme-ov-file#editable-installs), so all good! However, what do you want the dev version to look like?

  1. Bump minor tag (e.g., 0.5.0.dev20+g47f9643, currently implemented in this PR)
  2. Bump patch tag (e.g., 0.4.5.dev20+g47f9643.d20241014, the default)

@hofaflo
Copy link
Contributor

hofaflo commented Oct 14, 2024

Great, will do!

Finally, do you think we can skip

      - name: Install Python ${{ matrix.python-version }}
        run: uv python install ${{ matrix.python-version }}

and instead use

      - name: Pre-commit
        run: |
          uv run --extra dev --python ${{ matrix.python-version }} --with ${{ matrix.numpy }} pre-commit run --all-files
      - name: Pytest
        run: |
          uv run --extra dev --python ${{ matrix.python-version }} --with ${{ matrix.numpy }} pytest -Werror --cov --cov-report term-missing --cov-fail-under=100

?

I'd like to leave this as-is to keep the step commands shorter.

what do you want the dev version to look like?

  1. Bump minor tag (e.g., 0.5.0.dev20+g47f9643, currently implemented in this PR)

  2. Bump patch tag (e.g., 0.4.5.dev20+g47f9643.d20241014, the default)

Thanks for bringing this up! I think the default is more appropriate, as we aren't using release branches anyway.

@cbrnr cbrnr marked this pull request as ready for review October 15, 2024 06:58
@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 15, 2024

OK, I've switched to the default versioning scheme, I think this is now ready for review!

Copy link
Contributor

@hofaflo hofaflo left a comment

Choose a reason for hiding this comment

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

Looks perfect, thanks @cbrnr!

@hofaflo hofaflo merged commit b0420b5 into the-siesta-group:main Oct 15, 2024
11 checks passed
@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 15, 2024

Fingers crossed that everything works in the release workflow! But I guess we'll find out soon...

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 15, 2024

(Or maybe you test on test-pypi before the next release.)

@cbrnr cbrnr deleted the pypr branch October 15, 2024 07:25
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.

Consider a more standard build tool?
2 participants