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 pixi #802

Merged
merged 36 commits into from
Jun 25, 2024
Merged

Switch to pixi #802

merged 36 commits into from
Jun 25, 2024

Conversation

stanmart
Copy link
Collaborator

@stanmart stanmart commented May 30, 2024

This is a proposal to move from the current conda/mamba based dev installation to pixi. See #801 Check out the updated contributing page of the docs for info on how it changes the setup process.

Checklist

  • Added a CHANGELOG.rst entry

TODO:

  • Make CI work with pixi
    • Use the setup-pixi gh action
    • Add features with different python versions
    • Doctests
    • Pre-commit
  • Make daily runs work with pixi
    • See if tests against nightlies can be incorporated into an environment in pixi.toml
  • Platform dependent deps (e.g. intel-mkl for win)
  • Add other platforms to pixi.toml
  • Commit lockfile when we are happy with the basics
  • Figure out how to automatically update lockfile periodically (could be e.g. some weekly CI action)
  • Add environment with minimal dependencies that we want to support, and test against these "oldies"
  • Make it work with readthedocs

Advantages:

  • Can guarantee that local dev environment is the same for everyone (on the same platform), and also in CI
  • Can use platform-dependent requirements (e.g. intel-mkl for the benchmark env on non-arm archs)
  • Super fast
  • One file containing multiple environments, instead of having multiple environment*.yml files and having to do conda env updates to create certain environments.
  • Includes environments and tasks to test on oldest supported deps (oldies) and nightlies, both locally and in CI
  • There are pre-defined tasks for lots of commonly-used tasks (e.g. make and serve docs, install glum in editable mode, run tests).

Limitations:

  • Whille pixi supports editable installs, it cannot pass the necessary flags to pip/uv. This is the reason why we need the annoying postinstall task. (Not any worse than the current status quo, though.) Some proposed pixi features might help with this in the future:
  • pixi is still pre-1.0
  • Most people have some flavor of conda installed, but pixi is less widespread

@stanmart
Copy link
Collaborator Author

@pavelzw, let me know if you have any input regarding this PR or the general approach

@pavelzw
Copy link
Member

pavelzw commented May 31, 2024

Figure out how to automatically update lockfile periodically (could be e.g. some weekly CI action)

We already have this feature in our organization since last week.
An automatic PR comes every month and runs pixi update.

You can specify its behavior by setting the following in pixi.toml

[tool.update]
autoupdate-branch = "pixi-update"
autoupdate-commit-message = "Update pixi lockfile"
autoupdate-pull-request-labels = ["dependencies"]
autoupdate-schedule = "monthly" # weekly, monthly, quarterly, never
ignore-environments = []
ignore-platforms = []

@pavelzw
Copy link
Member

pavelzw commented May 31, 2024

See if tests against nightlies can be incorporated into an environment in pixi.toml

Would be interesting to see if/how this is supported the best way with pixi. Maybe running pixi remove dependency && pixi add --pypi --nightly(?) dependency in CI?

Copy link
Member

@pavelzw pavelzw left a comment

Choose a reason for hiding this comment

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

Efforts look good so far 👍

pixi.toml Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
@stanmart
Copy link
Collaborator Author

stanmart commented Jun 1, 2024

Thanks so much for the help @pavelzw! I think it is shaping up nicely. It has already reveled that our minimum pinned versions are not recent enough.

When we figure out how to handle nightlies (should be doable) and read the docs (less sure about this one) we should be good.

@pavelzw
Copy link
Member

pavelzw commented Jun 1, 2024

There is a more-or-less hacky way to do readthedocs with pixi, see https://github.com/prefix-dev/pixi/pull/1423/files#diff-65d574b20be487abb02f3d039096768be5613f955410494c7862d3f6b3781432 the readthedocs-override example

@stanmart
Copy link
Collaborator Author

stanmart commented Jun 1, 2024

See if tests against nightlies can be incorporated into an environment in pixi.toml

Would be interesting to see if/how this is supported the best way with pixi. Maybe running pixi remove dependency && pixi add --pypi --nightly(?) dependency in CI?

Atm I just use pip install to replace some packages with the nightly versions, without caring about their dependencies. This mirrors the current state of the daily ci runs. But yeah, would be interesting to see if we can do better in the future using pixi.

@stanmart
Copy link
Collaborator Author

stanmart commented Jun 1, 2024

There is a more-or-less hacky way to do readthedocs with pixi, see https://github.com/prefix-dev/pixi/pull/1423/files#diff-65d574b20be487abb02f3d039096768be5613f955410494c7862d3f6b3781432 the readthedocs-override example

Yeah it's indeed a bit hacky, but should be fine. I'm wondering if exporting the docs environment into a conda environment.yml, and using that for rtd would be a viable alternative. (I know that there is no pixi export command as of yet, but there seems to be one on the horizon, and the current pr is not urgent at all.)

@pavelzw
Copy link
Member

pavelzw commented Jun 1, 2024

Performance wise installing pixi with conda and then running pixi install is probably still faster 🤔

pixi.toml Outdated Show resolved Hide resolved
pixi.toml Show resolved Hide resolved
@stanmart
Copy link
Collaborator Author

There is one final question on my part about the mypy pre-commit hook. In the proposed version, we are running it in an environment where glum and all of its deps are installed. As a result, it finds lots of errors (most of which are not problematic ofc) even if only a tiny part of the source code changes. While this would be fine for a new project, it would require us to sprinkle type: ignores all over the code, and make sure mypy has no problems with even the unmodified parts of the codebase.

An alternative would be to move mypy into the lint environment, which would be analogous to the way things worked before (pre-commit's mypy in a separate virtual env, no access to imports). It would come up with fewer false positives at the cost of being able to catch fewer errors.

What should we do about this?

@stanmart
Copy link
Collaborator Author

@MatthiasSchmidtblaicherQC I like your idea about running just a subset of checks while a PR is draft (or the branch does not have an associated pr), but I feel that that is somewhat orthogonal to the current PR. I'd rather limit this PR to changing the package manager while keeping other things constant, and create a separate issue to improve the ci pipeline. Would that be okay with you?

@MarcAntoineSchmidtQC
Copy link
Member

MarcAntoineSchmidtQC commented Jun 12, 2024

@stanmart, I'm a bit confused. Maybe something that would help me right now is if you can point me to an example of a failed mypy run due to this new setup? Can I find it in the CI?

I ran pixi run pre-commit-run in the new environment, and I get a bunch of errors, but they are all internal to glum (not from the dependencies). Are those the errors you are talking about?

okay, I think I understand. You were talking about internal errors that were not discovered by mypy when we ran it in an isolated environment. So I think I'm getting the same errors. I'm looking into them. I think some are valid and helpful. Not sure why they were not caught before.

.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
@pavelzw
Copy link
Member

pavelzw commented Jun 12, 2024

Not sure why they were not caught before

before, mypy just skipped all import statements where the packages were not installed (because of

- --ignore-missing-imports
). in the pre-commit setup from before, mypy was installed in an isolated environment with nothing else in it.

now, mypy has these packages available (since it's installed in the default environment) and can actually check whether the types are correct or not

@MarcAntoineSchmidtQC
Copy link
Member

@stanmart, I added two commits. One to fix the "exclude" option of mypy which needs to be specified in the pre-commit-hook file and not in the config (a quirk of pre-commit). The other to fix the mypy errors. I get all green on my end when running locally.

@MarcAntoineSchmidtQC
Copy link
Member

MarcAntoineSchmidtQC commented Jun 12, 2024

Final point: I think with this new setup we may need to run pre-commit run --all, and not simply pre-commit run. My understanding of the github action (which we previously used) is that it was running on all files. When running as a pre-commit hook, it only runs on changes, but the CI runs on everything.

Fixing this should run the pre-commit checks in the CI instead of skipping them. See here for example.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@pavelzw
Copy link
Member

pavelzw commented Jun 12, 2024

One to fix the "exclude" option of mypy which needs to be specified in the pre-commit-hook file and not in the config (a quirk of pre-commit).

It's true that you need to add it to .pre-commit-config.yaml but you might want to reconsider removing the exclude from pyproject.toml (I know, it's bad to repeat oneself) because this will lead to pixi run mypy . behaving differently than running mypy within the pre-commit hook.

# mypy
- id: mypy
name: mypy
entry: pixi run -e default mypy --check-untyped-defs --ignore-missing-imports --namespace-packages
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
entry: pixi run -e default mypy --check-untyped-defs --ignore-missing-imports --namespace-packages
entry: pixi run -e default mypy

I would suggest moving --check-untyped-defs and --namespace-packages to the pyproject.toml confiiguration and removing --ignore-missing-imports altogether as it can lead to issues.

Instead, I would suggest installing additional types if needed (like pandas-stubs, types-psutil, ...) to your test feature next to mypy and for dependencies that don't have stubs yet adding an exception to pyproject.toml as follows:

[[tool.mypy.overrides]]
# https://github.com/scikit-learn/scikit-learn/issues/16705
module = ["sklearn.*"]
ignore_missing_imports = true

@stanmart
Copy link
Collaborator Author

Thanks, @MarcAntoineSchmidtQC and @pavelzw! I think most of your comments are now addressed. Later this week I'll add the type stub packages for dependencies that have them, and then try removing ignore_missing_imports.

@MarcAntoineSchmidtQC
Copy link
Member

Thanks, @MarcAntoineSchmidtQC and @pavelzw! I think most of your comments are now addressed. Later this week I'll add the type stub packages for dependencies that have them, and then try removing ignore_missing_imports.

This seems to be rather tedious (I tried removing the flag and there are a lot of missing packages) because most of them will need there specific ignore config. I would prefer if we merged this first, and left this as a future task.

@stanmart
Copy link
Collaborator Author

stanmart commented Jun 13, 2024

Sounds good! Maybe we can still add some stub libraries so we get some extra checks, but keep ignore_missing_imports to ignore the rest for now.

Other than that case this PR looks ready to merge to me. Maybe a final question: do we need a changelog entry for this?

@jtilly
Copy link
Member

jtilly commented Jun 15, 2024

Before merging this, we need to update the docs (e.g., https://glum.readthedocs.io/en/latest/contributing.html#install-for-development) and explain how to use this new machinery.

@stanmart
Copy link
Collaborator Author

stanmart commented Jun 15, 2024

I added some instructions a while ago, but it can be expanded a bit (e.g. with how to install both glum and tabmat in editable mode). Are there any other files that we need to update?

docs/contributing.rst Outdated Show resolved Hide resolved
@MarcAntoineSchmidtQC
Copy link
Member

@stanmart, was there something else you wanted to do in this PR? Happy to merge whenever you are okay with it.

@stanmart
Copy link
Collaborator Author

I think we can merge this as it is.

@MarcAntoineSchmidtQC MarcAntoineSchmidtQC merged commit 2684b86 into main Jun 25, 2024
22 checks passed
@MarcAntoineSchmidtQC MarcAntoineSchmidtQC deleted the switch-to-pixi branch June 25, 2024 14:40
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.

Use pixi for setting up the dev environment
5 participants