Skip to content

Conversation

@jokasimr
Copy link
Contributor

@jokasimr jokasimr commented Jul 14, 2025

Fixes scipp/plopp#463

Suggested solution based on comment by @jl-wynen

- uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python.version }}
- run: python -m pip install --upgrade uv pytest
Copy link
Member

Choose a reason for hiding this comment

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

[minor]
[test] environment should already have pytest... I think...? So maybe no need to install pytest here.

Copy link
Member

@YooSunYoung YooSunYoung left a comment

Choose a reason for hiding this comment

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

👍

ESS_PROTECTED_FILESTORE_PASSWORD: ${{ secrets.ESS_PROTECTED_FILESTORE_PASSWORD }}
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
Copy link
Member

Choose a reason for hiding this comment

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

Better use https://github.com/astral-sh/setup-uv It's a bit odd to run uv from within a python env. (Although possible)

You should then be able to use

- run: uv run --extra=test --resolution=lowest-direct pytest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's nice, thanks

@jokasimr jokasimr requested a review from jl-wynen July 15, 2025 07:43
@jokasimr
Copy link
Contributor Author

jokasimr commented Jul 15, 2025

Right now we typically don't have lower bounds on our dependencies. To make the lower bound tests pass we need to set some lower pin on most of our dependencies because otherwise some of them resolve to something ridiculous like 0.0.1-alpha-use-at-your-own-peril.

It's hard to find the true "lower bound" combination of all dependencies that passes the tests (an interesting problem though) but in practice I think it makes sense to just set all bounds to something like the latest release at least 1-2 years old, and if it doesn't pass the tests there then manually fiddle until the tests pass.

For example, here is a combination of lower pins I found that lets the essdiffraction tests pass:

dependencies = [
  "dask>=2024.7.0",
  "essreduce>=25.05.3",
  "graphviz",
  "numpy>=1.25",
  "plopp>=25.03.0",
  "pythreejs>=2.4.1",
  "sciline>=25.04.1",
  "scipp>=25.05.1",
  "scippneutron>=25.02.0",
  "scippnexus>=23.12.0",
  "tof>=25.01.2",
]

[project.optional-dependencies]
test = [
    "pandas>=2.2.2",
    "pooch>=1.5",
    "pytest>=7.0",
    "ipywidgets>=7.8.2"
]

@jl-wynen
Copy link
Member

Interesting, that graphviz has no lower bound. So we can use the initial release?

in practice I think it makes sense to just set all bounds to something like the latest release at least 1-2 years old, and if it doesn't pass the tests there then manually fiddle until the tests pass.

That sounds reasonable. We don't need to support multi-year-old dependencies. SO just picking a version as you described should work well.

Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

Did you deploy this to a project and ran CI?

@jokasimr
Copy link
Contributor Author

jokasimr commented Jul 15, 2025

Did you deploy the project and ran the CI

No I just tested it locally first.

I'll try it in the CI in a few minutes, see scipp/essdiffraction#189.

@jokasimr
Copy link
Contributor Author

Interesting, that graphviz has no lower bound. So we can use the initial release?

It might be constrained by other dependencies already, for example maybe sciline has a lower pin. I'm not sure.

@jokasimr
Copy link
Contributor Author

jokasimr commented Jul 15, 2025

The workflow ran in essdiffraction: https://github.com/scipp/essdiffraction/actions/runs/16293424662/job/46008694444

(the regular tests fail there, but that is unrelated)

@jokasimr jokasimr requested a review from jl-wynen July 15, 2025 12:47
Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

Can you move the job into a separate file as discussed on Slack?

@jokasimr jokasimr requested a review from jl-wynen July 15, 2025 14:11
@jokasimr jokasimr merged commit 3561fcd into main Jul 15, 2025
2 checks passed
@jokasimr jokasimr deleted the lower-bound-nightly branch July 15, 2025 15:29
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.

Add minimum Scipp requirement?

4 participants