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

ci: Update CI to use Pixi #971

Merged
merged 31 commits into from
Nov 12, 2024
Merged

ci: Update CI to use Pixi #971

merged 31 commits into from
Nov 12, 2024

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Oct 14, 2024

Changes the developer/CI from Hatch to Pixi. This will align it with our other repos.

Supersede #970

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.14%. Comparing base (0d191dc) to head (e619273).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
+ Coverage   86.45%   87.14%   +0.69%     
==========================================
  Files          10        9       -1     
  Lines        5177     4926     -251     
==========================================
- Hits         4476     4293     -183     
+ Misses        701      633      -68     

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

@@ -1,41 +1,189 @@
# Developer guide
# Developer Guide
Copy link
Member Author

Choose a reason for hiding this comment

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

This has been copied from Panel. I should have updated it to match the param, but there could potentially be something I haven't updated correctly.

@@ -1,7 +1,7 @@
# This is the configuration for pre-commit, a local framework for managing pre-commit hooks
# Check out the docs at: https://pre-commit.com/

default_stages: [commit]
default_stages: [pre-commit]
Copy link
Member Author

Choose a reason for hiding this comment

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

Stop emitting a warning.

@@ -717,8 +717,8 @@
"class P(param.Parameterized):\n",
" p = param.Path('Parameter_Types.ipynb')\n",
" f = param.Filename('Parameter_Types.ipynb')\n",
" d = param.Foldername('lib', search_paths=['/','/usr','/share'])\n",
" o = param.Filename('output.csv', check_exists=False)\n",
" d = param.Foldername('lib', search_paths=['/','/usr','/share'], check_exists=False)\n",
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes made in this file are so we can run this notebook on Windows, too.

@@ -39,13 +40,7 @@ examples = [
"pandas",
"panel",
]
doc = [
Copy link
Member Author

@hoxbro hoxbro Nov 7, 2024

Choose a reason for hiding this comment

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

I have removed the developer-focused optional dependencies doc and lint, also coverage.

@@ -590,7 +590,7 @@ def gen():
async def test_reactive_gen_pipe():
def gen(val):
yield val+1
time.sleep(0.05)
time.sleep(0.1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid the flaky Windows test; use the same fix as #967.

@hoxbro hoxbro marked this pull request as ready for review November 7, 2024 11:44
@hoxbro hoxbro requested a review from maximlt November 7, 2024 11:49

jobs:
waiting_room:
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean I'll have to click somewhere to release Param? I don't think I've done it yet for other places where you added that. If so, am I supposed to check something specific before I click?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It is done on the other repos, though I have limited it to lead maintainers (and me).

I use it to double-check that the version tag is correct.

Copy link
Member

Choose a reason for hiding this comment

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

I have limited it to lead maintainers (and me).

Ok. Where is the least of lead maintainers defined?

I use it to double-check that the version tag is correct.

Too bad we no longer trust the system to produce the right version tag, but that's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Too bad we no longer trust the system to produce the right version tag, but that's fine.

The "system" is writing tags ourselves and git push (ref). PyPi also strongly encourages it (ref).

Copy link
Member

Choose a reason for hiding this comment

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

PyPi also strongly encourages it (ref).

I'm sorry I don't understand, this page links to using a trusted publisher. Are we doing that?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we are in places (though maybe not everywhere yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry I don't understand, this page links to using a trusted publisher. Are we doing that?

Just to be sure:

  • The waiting room job itself is to ensure that all the building steps succeed. For example, if the PyPi build step fails, the conda package will not be released.
  • Inside the waiting room is an environment publish, which people must be approved before release. This was what I tried to reference in the PyPI ref.
    image

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks I got it.

The waiting room job itself is to ensure that all the building steps succeed. For example, if the PyPi build step fails, the conda package will not be released.

It sounds like it's more for manually checking the two builds, since otherwise it could be automated.

Inside the waiting room is an environment publish, which people must be approved before release. This was what I tried to reference in the PyPI ref.

Ok I understand better how you use the environment to add this protection rule. Seems not so related to the trusted publisher thing for which environment is optional but permissions with id-token: write is not.

image

By the way I think you should add Jean-Luc and we probably need to update the list of maintainers.

image

All of that sounds like it would be worth being documented somewhere, it's kind of hard to follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

manually checking the two builds

It is automatically done. If the environment were not there, it would do everything without interaction.

All of that sounds like it would be worth being documented somewhere

Yes. I will update the holoviz site (soon).

- name: build docs
run: hatch -v run docs:build

docs_publish:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this workflow divided in two jobs?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the upload fails because of a connection issue, you don't need to build the docs again. However, it is not a big problem in param.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I've never seen that fail before during a docs build. Do you mind simplifying this workflow into one single job? Let's keep things simple!

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep things simple!

I agree, though I think the simplest thing is to align the CI files across projects.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder to which extent this is needed for repositories that a low development activity like Param. I guess it doesn't hurt so much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it doesn't hurt. Some reasons, albeit not the most significant problem from param:

  • Cache is not shared between PRs. So if no lock is run on the main branch, each PR needs to lock it, the first time.
  • Away to get a snapshot of the environment. This can be used to see what package caused a failure. It could also be used to see how the param environments looked a year ago.

Copy link
Member

Choose a reason for hiding this comment

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

Away to get a snapshot of the environment. This can be used to see what package caused a failure. It could also be used to see how the param environments looked a year ago.

Ah yes I missed that the lock file was pushed to S3 too. Sounds useful, when you know about it ;)

environments: ${{ matrix.environment }}
- name: Test Unit
run: |
pixi run -e ${{ matrix.environment }} test-unit $COV
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a test-unit-cov command? This is also useful to run as a developer and not just for the CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't needed to run coverage locally, but you can also run it with pixi run test-unit --cov=param.

I'm cautious about adding more tasks, which could confuse new contributors.

Copy link
Member

Choose a reason for hiding this comment

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

I liked when you said that you moved as much as possible from the config that was on the CI workflows to the pixi config file (e.g. env vars) which is why I was a little surprised to see that there in test.yaml.

I'm cautious about adding more tasks, which could confuse new contributors.

My experience so far has been to copy/paste the commands displayed in the developer guide. I have not relied on pixi task list which I think just gives a simple list of tasks available without their definition, and that's pretty cluttered already anyway.

I'm fine with how things are anyway here.

pytest = "*"
pytest-asyncio = "*"
pytest-cov = "*"
pytest-github-actions-annotate-failures = "*"
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of this plugin. It's also definitely not part of the core deps needed to tun the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a big fan of this plugin.

I'm a big fan of it. Reduces the need to look into the logs.

It's also definitely not part of the core deps needed to tun the tests.

The same could be set about pytest-cov. The plugin is a no-op if you are not in a Github action and only have pytest as a dependency.

The naming is maybe not the best. A correct name would be test-tools or test-deps — dependencies needed to run the unit tests in every configuration.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of it. Reduces the need to look into the logs.

My experience of it so far has been to give me the impression that, basically, every Panel PR is failing as it also displays errors that occur during flaky tests.


The same could be set about pytest-cov. The plugin is a no-op if you are not in a Github action and only have pytest as a dependency.

Reminds me of the discussion we recently had about hatch-conda-build :D pytest-cov can be used by a user, I don't think pytest-github-actions-annotate-failures can?

But anyway, I'm again fine with how this is.

Copy link
Member Author

Choose a reason for hiding this comment

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

My experience of it so far has been to give me the impression that, basically, every Panel PR is failing as it also displays errors that occur during flaky tests.

Yes. There is an open issue for it here pytest-dev/pytest-github-actions-annotate-failures#30.

I think the frustration of it showing up a lot in Panel PRs is a bit misplaced. The correct thing to be frustrated about is the flaky test themselves 🙂

It's not a problem for Param as it doesn't have flaky tests.

Copy link
Member

Choose a reason for hiding this comment

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

The correct thing to be frustrated about is the flaky test themselves 🙂

Both things are frustrating.


python -m build -s .

VERSION=$(python -c "import $PACKAGE; print($PACKAGE._version.__version__)")
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, is there a reason this is not PACKAGE.__version__?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to be sure that the automatic version of the file is generated. param.__version__ is more "complex" with try/except.

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Except a typo and a simplification I'd like to see in the docs workflow, I'm fine with these changes.

@hoxbro I would strongly encourage you to update the HoloViz operating guide (https://holoviz.org/contributing.html#operating-guide) now that the workflows you created are used in the majority of places and pyctdev is no longer used in the main repositories. There are many behaviors and explanations as to why things are how they are that are just in your mind and that would be worth sharing with others, current and future contributors/maintainers. If you have custom tooling that is used to inspect/interact with the system you created, it would also be nice to share it in a HoloViz repo.

Co-authored-by: Maxime Liquet <[email protected]>
Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Thanks @hoxbro for doing this work! I'm going to get this merged now as I'd like the next release to use this new infrastructure.

I don't necessarily agree with the increased complexity some workflows got (e.g. publishing waiting room, docs build in two jobs) but I'm not the one doing all the hard work setting up this infra and maintaining it across HoloViz, so it's alright!!

@maximlt maximlt merged commit 76c7981 into main Nov 12, 2024
16 checks passed
@maximlt maximlt deleted the pixi branch November 12, 2024 10:37
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.

3 participants