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 project to uv #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Migrate project to uv #99

wants to merge 3 commits into from

Conversation

AlinderS
Copy link
Collaborator

Add a pyproject.toml with project dependencies.
Dependencies are now in three groups.

  1. Core packages. These are required regardless of what you want to do. Example: Numpy.
  2. Tools. Packages that are only needed to run the examples in presentation 4. Since there are so many of these, splitting them out should make maintaining easier.
  3. Dev. Pre-commit is only needed if you want to work on the code so it gets a separate category.

Notably, the original RISE is not included because it is not compatible with JupyterLab or Jupyter notebook > 6. Instead, we use jupyterlab-rise. This has its own issues, most notably that when executing the last cell on a slide, the presentation will advance. I have tried the workaround proposed here which is to insert an empty cell of type skip at the end of each affected slide. I found that it works consistently but another PR will probably be needed to add these to all slides since the issue does not seem to be worked on.

Remove Pipfile and add uv.lock.
I also updated the pre-commit hooks since I was messing with it anyway.

pyproject.toml Outdated
[project]
name = "lundpython"
version = "0.1.0"
description = "Add your description here"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't keep the default here.

Comment on lines +6 to +13
requires-python = ">=3.12"
dependencies = [
"astropy>=7.0.0",
"jupyter>=1.1.1",
"jupyterlab-rise>=0.42.0",
"matplotlib>=3.9.2",
"numpy>=2.1.3",
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a good reason for not complying with SPEC 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What part of this is in violation of SPEC 0?
It seems to me the spec is about not supporting or requiring old versions of packages, and all these are new.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Specifying how long specific versions of dependencies should be supported is the entire point of the SPEC:

Specifically, we recommend that:

  1. Support for Python versions be dropped 3 years after their initial release.
  2. Support for core package dependencies be dropped 2 years after their initial release.

Although here it makes sense to support versions that will be 2 years old next autumn instead of right now.


[dependency-groups]
dev = [
"pre-commit>=4.0.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't write down pre-commit as a dependency here. On one hand, listing it as a dependency is not good enough because that just installs the framework, but the hooks still need to be installed in a separate manual step anyways. On the other hand, there aren't any benefits to isolating pre-commit because the framework sets up separate environments for each of the hooks itself.

dev = [
"pre-commit>=4.0.1",
]
tools = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a one-sentence comment to explain why these are separate.

uv.lock Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of locking down the dependencies? Our instructions should not critically depend on exact versions students might have installed. It's not at all obvious why specifying the minimal supported versions on pyproject.toml would not be good enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I included the lock file because that is the default recommended behaviour of uv, but I agree that we don't require that strict control so it is better to leave it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm willing to guess that the uv recommendation relies on the assumption that the code is tested using different versions of dependencies. But this project doesn't even have a test suite, so if a lock file forces the few people who are involved with this to use exactly the same versions of dependencies then some problems might go unnoticed for a long time.

Also update pre-commit hooks versions.
It has been replaced with pyproject.toml
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