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

Run poetry update and reinstall everything with pip #20

Merged
merged 7 commits into from
Nov 19, 2024

Conversation

hugobuddel
Copy link
Contributor

This should prevent ScopeSim_Data from downgrading scopesim to 0.9.0 or whatever all the time.

@hugobuddel
Copy link
Contributor Author

Draft because AstarVienna/ScopeSim#503 should first be merged, and then the slack bot should be turned on again. But conceptually this is ready.

Copy link
Contributor

@teutoburg teutoburg left a comment

Choose a reason for hiding this comment

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

Yeah, all the pip install -e <pkg> at the end are probably the best way to solve this now. That's what I have been doing locally anyway when testing if e.g. a change in ScopeSim would break an IRDB notebook. That is, have the local feature branch with that change checked out and running pip install -e, then run the notebooks.

Now, the question is, why do we still need the poetry commands in this script at all??

@hugobuddel
Copy link
Contributor Author

Now, the question is, why do we still need the poetry commands in this script at all??

Because it is not possible to install the dependency groups without poetry..

See https://stackoverflow.com/questions/76118614/is-it-possible-to-install-poetry-groups-with-pip

But that page links to https://discuss.python.org/t/-/39233

and that links to https://peps.python.org/pep-0735/ "Dependency Groups in pyproject.toml", which is marked as Accepted, so maybe there will be a better way to get those dependency groups in soon

@teutoburg
Copy link
Contributor

Now, the question is, why do we still need the poetry commands in this script at all??

Because it is not possible to install the dependency groups without poetry..

Ah right, I remember we've been there...

and that links to https://peps.python.org/pep-0735/ "Dependency Groups in pyproject.toml", which is marked as Accepted, so maybe there will be a better way to get those dependency groups in soon

Looks useful, ideally poetry would incorporate that once it's actually a thing, then we would get the best of both worlds...

@hugobuddel hugobuddel changed the title Run poetry lock and reinstall everything with pip Run poetry update and reinstall everything with pip Nov 19, 2024
@hugobuddel
Copy link
Contributor Author

hugobuddel commented Nov 19, 2024

For some reason this is still

  - Installing nbclient (0.9.0)

and then

   - Downgrading nbclient (0.9.0 -> 0.5.13)

While this does not work. Version 0.10.0 of nbclient should work

@hugobuddel
Copy link
Contributor Author

Apparently doing poetry install followed by poetry update gives a different (worse) result than doing poetry lock and then poetry install..

ScopeSim_Templates$ poetry shell
ScopeSim_Templates$ pip list | grep jupyter_client
# Nothing
ScopeSim_Templates$ poetry install --with=test,dev,docs
ScopeSim_Templates$ pip list | grep jupyter_client
jupyter_client                7.4.9
ScopeSim_Templates$ pip list | grep jupyter_client
jupyter_client                7.4.9

Oh FFS, rubber ducking by writing this comment, I realized I could try to add --with to poetry update, and indeed that works:

ScopeSim_Templates$ poetry update --with=test,dev,docs
ScopeSim_Templates$ pip list | grep jupyter_client
jupyter_client                8.6.3

But this --with is not required for poetry lock.

Maybe it's just me, but I find poetry so much more unintuitive than e.g. conda. Why would I only want to update the dependencies of certain groups?

@hugobuddel
Copy link
Contributor Author

P.R. created to update the poetry documentation to save other people the trouble: python-poetry/poetry#9857

@hugobuddel
Copy link
Contributor Author

Thinking more about this, we are still doing things wrong. Say speXtra has a weird test dependency, that is not required by ScopeSim, then that dependency would be removed when installing ScopeSim, thereby removing the ability to test speXtra.

Instead of installing all the packages, ScopeSim_Data should maybe be its own poetry project, and then this CI job should add all dependencies through

poetry add "git+https://github.com/AstarVienna/ScopeSim@main[dev,test,docs]"

(or maybe through the cloned local path with -E) and then install ScopeSim_Data. Or something like that.

Or we should finally move to ScopeSim_Core (the library) and have ScopeSim (the application) have all possible relevant dependencies as optional dependency groups, then it is possible to just install that. Hmm no that won't work either, because then say that one of the packages has a new dependency in the test group, but the package is not yet released itself, how to get that in.

@hugobuddel hugobuddel marked this pull request as ready for review November 19, 2024 11:10
@hugobuddel
Copy link
Contributor Author

This should prevent the failure of the last nights because AstarVienna/ScopeSim#503 has been fixed on main, and this P.R. now actually allows ScopeSim_Data to test with the main branch of all our projects.

@teutoburg
Copy link
Contributor

Instead of installing all the packages, ScopeSim_Data should maybe be its own poetry project

Yeah that came to my mind as well...

@teutoburg teutoburg self-requested a review November 19, 2024 13:50
Copy link
Contributor

@teutoburg teutoburg left a comment

Choose a reason for hiding this comment

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

It's certainly better than it was before in terms of doing what we want this test to do.

@hugobuddel hugobuddel merged commit ebe7116 into main Nov 19, 2024
@teutoburg teutoburg deleted the hb/reinstallwithpip branch November 19, 2024 14:09
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