Skip to content

Conversation

@orcahmlee
Copy link
Contributor

@orcahmlee orcahmlee commented Dec 2, 2025

Related to #37, this PR had two changes:

  • 9f53c76: The pyproject.toml was updated to use uv. All of the settings were migrated from poetry.
  • f5258f0: Updated the usage of the uv command for installing and building, and also retained the pip command.

Here are some manual validations I did:

  • Created a new virtual environment using uv venv and installed the dependencies using uv pip install -e ., uv pip install -e ".[aioldap]" and uv pip install -e ".[test]"
  • Built the Python package by uv build
  • Ran the unit test by uv run pytest

@orcahmlee
Copy link
Contributor Author

Hi @gstein , @sbp , @tisonkun

This is the first trial, please give me some advice.

@shenxiangzhuang
Copy link

shenxiangzhuang commented Dec 3, 2025

I think the workflow should also be changed to use uv, now it's still using poetry:

- name: Test with pytest
run: |
poetry run pytest -rA

@orcahmlee
Copy link
Contributor Author

I think the workflow should also be changed to use uv, now it's still using poetry:

- name: Test with pytest
run: |
poetry run pytest -rA

Seems the entire workflow using poetry. I need more time to figure out Github Workflows 😢

fetch-depth: 0 # need all versions to be able to access server tree commit logs
persist-credentials: false
- name: Install uv and set the Python version
uses: astral-sh/setup-uv@v7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @shenxiangzhuang

Would you mind reviewing this change where I use the uv official actions to set up Python and uv itself.
Ref: https://docs.astral.sh/uv/guides/integration/github/

Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep the original caching behavior, we could try https://github.com/astral-sh/setup-uv/blob/main/docs/caching.md. But if the CI isn't run as often, I think we're good to keep it as is.

with:
python-version: ${{ matrix.python-version }}
enable-cache: true # enable built-in caching of uv
- uses: awalsh128/cache-apt-pkgs-action@5902b33ae29014e6ca012c5d8025d4346556bd40 # latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is not poetry related so I kept it.

fetch-depth: 0 # need all versions to be able to access server tree commit logs
persist-credentials: false
- name: Install uv and set the Python version
uses: astral-sh/setup-uv@v7
Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep the original caching behavior, we could try https://github.com/astral-sh/setup-uv/blob/main/docs/caching.md. But if the CI isn't run as often, I think we're good to keep it as is.

README.md Outdated

```shell
poetry run pytest
uv sync --group test
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uv sync --group test
uv sync --extra aioldap --group test

if the test also cover the extra, this would be needed. If not, we should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
I guess the test should cover all extras including the further extras. Furthermore, since I install all extras during the Action, I think it would be better to maintain consistency in the README.

      - name: Install dependencies
        run: uv sync --locked --all-extras --group test

@tisonkun
Copy link
Member

tisonkun commented Dec 4, 2025

@gstein it seems we need a member with write permission on this repo to trigger the CI workflow.

@orcahmlee or else, you can open a PR to your fork with your fork updated to the latest main. Then you can verify the CI workflow on your repo PR as well.

@orcahmlee
Copy link
Contributor Author

@gstein it seems we need a member with write permission on this repo to trigger the CI workflow.

@orcahmlee or else, you can open a PR to your fork with your fork updated to the latest main. Then you can verify the CI workflow on your repo PR as well.

I've rebased the latest upstream/main then created a PR on my own repo. Perhaps I miss something setting for GitHub Actions, I didn't see the CI workflow.

orcahmlee#1

@orcahmlee
Copy link
Contributor Author

@gstein it seems we need a member with write permission on this repo to trigger the CI workflow.

@orcahmlee or else, you can open a PR to your fork with your fork updated to the latest main. Then you can verify the CI workflow on your repo PR as well.

Manually trigger the CI workflow on my repo. Seems only the test failed in Python 3.14
https://github.com/orcahmlee/infrastructure-asfquart/actions/runs/19919472414/job/57105098829#step:6:102

@orcahmlee
Copy link
Contributor Author

@gstein it seems we need a member with write permission on this repo to trigger the CI workflow.

@orcahmlee or else, you can open a PR to your fork with your fork updated to the latest main. Then you can verify the CI workflow on your repo PR as well.

What should I do next? Should I remove the unit test in Python 3.14?

@tisonkun
Copy link
Member

tisonkun commented Dec 6, 2025

@gstein it seems we need a member with write permission on this repo to trigger the CI workflow.
@orcahmlee or else, you can open a PR to your fork with your fork updated to the latest main. Then you can verify the CI workflow on your repo PR as well.

What should I do next? Should I remove the unit test in Python 3.14?

Yes, you can and leave it as a follow up. IIRC Python 3.14 is not included in the test matrix now.

I'll try to give it a review tomorrow and hopefully @gstein would have a look also.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Generally looks good. I think we have two remaining tasks before this patch gets mergeable:

  1. The Makefile needs to be updated to avoid poetry deps.
  2. I'd prefer a RELEASE.md file to describe how to release asfquart with uv setup so that we can immediately follow up use a 0.1.14 release to verify it. Or you may describe it in this PR first.

@orcahmlee
Copy link
Contributor Author

orcahmlee commented Dec 7, 2025

Generally looks good. I think we have two remaining tasks before this patch gets mergeable:

  1. The Makefile needs to be updated to avoid poetry deps.
  2. I'd prefer a RELEASE.md file to describe how to release asfquart with uv setup so that we can immediately follow up use a 0.1.14 release to verify it. Or you may describe it in this PR first.
  1. Done
  2. I'm not quite sure what the content of the RELEASE.md should be. If my understanding is correct that I think the release process could be:
    uv build
    # The source distributions and wheels will be built to the `dist` directory
    
    uv publish
    # This command will upload the distributions and wheels in the `dist` directory to the `https://upload.pypi.org/legacy/` index as default
    # Pass the username/password or token

If I have misunderstood, could you give some examples?

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM.

About RELEASE.md, I suppose @gstein or an INFRA member can try your commands out and once it's verified, we can provide a proper RELEASE.md following any convenient pattern.

@orcahmlee
Copy link
Contributor Author

LGTM.

About RELEASE.md, I suppose @gstein or an INFRA member can try your commands out and once it's verified, we can provide a proper RELEASE.md following any convenient pattern.

@tisonkun , @Lee-W Thanks for your reviews.

@gstein gstein merged commit db66de6 into apache:main Dec 8, 2025
4 checks passed
@gstein
Copy link
Member

gstein commented Dec 8, 2025

Thanks for the PR. Very helpful!

@tisonkun
Copy link
Member

tisonkun commented Dec 8, 2025

@orcahmlee Thanks for your contribution and continued follow-up!

If you have some spare time and are interested in how ASF organized its "internal" services, you may take a look at Apache STeVe that runs most foundation-wide votings in the ASF. (We'll use it to run the vote in the upcoming membership meetings.)

Besides, you may also dig into why Python 3.14 failed the CI to add its support.

Take these suggestions as advice rather than requests - Just enjoy hacking in the community.

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.

5 participants