search

mxcube / mxcubeweb

MXCuBE-Web
http://mxcube.github.io/mxcubeweb/
GNU Lesser General Public License v3.0
25 stars 39 forks source link

CI failing on poetry-export #1473

Closed marcus-oscarsson closed 1 month ago

marcus-oscarsson commented 1 month ago

The CI is failing on poetry-export, it seems that we now need to install this plugin explicitly ?

poetry-export............................................................Failed
- hook id: poetry-export
- files were modified by this hook

Warning: poetry-plugin-export will not be installed by default in a future version of Poetry.

fabcor-maxiv commented 1 month ago

it seems that we now need to install this plugin explicitly ?

We already do: https://github.com/mxcube/mxcubeweb/pull/1269/files#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9R38.

Where does it fail? This one https://github.com/mxcube/mxcubeweb/actions/runs/11549893904/job/32143790326?pr=1471?

fabcor-maxiv commented 1 month ago

So, it should not show the warning. The issue has been reported: https://github.com/python-poetry/poetry-plugin-export/issues/249.

marcus-oscarsson commented 1 month ago

Here: https://github.com/mxcube/mxcubeweb/actions/runs/11527304882/job/32092840490

On second thought I don't think its the plugin install that fails but that this will always fail on dependabot updates right ?

walesch-yan commented 1 month ago

Yes, this one https://github.com/mxcube/mxcubeweb/actions/runs/11549893904/job/32143790326?pr=1471 failed due to poetry-export, but only after rebasing it to the newest changes in mxcubeweb

fabcor-maxiv commented 1 month ago

On second thought I don't think its the plugin install that fails but that this will always fail on dependabot updates right ?

Oh... Yeah... That might be the issue...

I need to think about it.

walesch-yan commented 1 month ago

Here: https://github.com/mxcube/mxcubeweb/actions/runs/11527304882/job/32092840490

On second thought I don't think its the plugin install that fails but that this will always fail on dependabot updates right ?

I think this was actually the issue, since we did not bump the version of werkzeug in the requirements.txt file.

Yes, this one https://github.com/mxcube/mxcubeweb/actions/runs/11549893904/job/32143790326?pr=1471 failed due to poetry-export, but only after rebasing it to the newest changes in mxcubeweb

This one then failed because I did not re-run the pre-commits after the rebase, I tried it again with the needed changes for requirements.txt and passed the tests.

walesch-yan commented 1 month ago

I can make a PR with the changes to requirements.txt (I guess this should not depend on other PRs) to fix this specific issue, but as you mentioned we might need a solution for future dependabot updates.

fabcor-maxiv commented 1 month ago

https://github.com/mxcube/mxcubeweb/pull/1472 was merged although the CI pipeline was not fully green. Nothing should be merged if the CI pipeline is not 100% green. I believe we should put the rules on the branches to prevent this from happening. Anyone knows how to do this on GitHub?

marcus-oscarsson commented 1 month ago

Its already the case @fabcor-maxiv, I force merged because I thought it was the warning that was the issue and it was just a patch version bump. I wont do it on the future. However I'm a bit reluctant to do a manual rebase on dependabot PRs

walesch-yan commented 1 month ago

One Idea would also be to create a workflow that is specifically triggered when dependabot makes a push to explicitly run poetry export before the tests. That way, we do not have to manually rebase on its PRs.

fabcor-maxiv commented 1 month ago

Its already the case @fabcor-maxiv, I force merged because I thought it was the warning that was the issue and it was just a patch version bump. I wont do it on the future. However I'm a bit reluctant to do a manual rebase on dependabot PRs

Ah ok, I see. Yes, I understand that having to manually modify the Dependabot pull requests would be annoying... :/

On the other hand: I added the auto-generated requirements.txt because I had understood that some facilities might want to use it to install with pip (because they don't install with Poetry into production, and thus miss on the lock-file and the guarantees of pinned dependencies). But we should ask if anyone actually does use this requirements.txt file, otherwise we can remove the pre-commit hooks.

One Idea would also be to create a workflow that is specifically triggered when dependabot makes a push to explicitly run poetry export before the tests. That way, we do not have to manually rebase on its PRs.

Ah yes, that could be a solution, needs investigating...

rhfogh commented 1 month ago

@fabcor-maxiv Ah, so it would be possible to install without poetry - avoiding the mess with poetry.lock complaining whenever you change something? That would be great. Is it documented anywhere?

marcus-oscarsson commented 1 month ago

Sounds like a good idea to ask who is using the generated requirements.txt, we are installing with poetry for development and pip for packages so we are not using the auto-generated requirements.txt.

fabcor-maxiv commented 1 month ago

@fabcor-maxiv Ah, so it would be possible to install without poetry - avoiding the mess with poetry.lock complaining whenever you change something? That would be great. Is it documented anywhere?

https://mxcubeweb.readthedocs.io/en/latest/dev/deployment.html#install-without-using-poetry

But I wonder what "mess with poetry.lock" you are taking about. Most of the pain should now be taken care of by the pre-commit hooks, at least on mxcubeweb (I do not know how it is on mxcubeqt).

we are installing with poetry for development and pip for packages so we are not using the auto-generated requirements.txt.

So you are using pinned dependencies for development and unpinned dependencies for production?

Well, we have the same issue and I have been meaning to fix this in our deployment procedure, but I always get sidetracked... My point is that dependencies should preferably be pinned when deploying to production.

marcus-oscarsson commented 1 month ago

We are also still elaborating our deployment procedure and we are tagging and building a package for each release. However we do a install with poetry from the tag corresponding to the release. One of the reasons is that poetry has a lock file, and that at the moment we still find it the most practical way to perform local fixes quickly.

rhfogh commented 1 month ago

@fabcor-maxiv What I would find easier to understand would be having a conda-environment.yml or a requirements.txt that describes the dependencies. When you install you use those. If you want to make a change, you modify them and reinstall. It is transparent what you have and what you get. I keep getting into the situation where I make some modification (mostly commenting out the mxcubecore dependency, since I want to use the repository I have, not something poetry downloads from somewhere) , and then having to do 'poetry lock --no-update' to be able to continue.. When you are doing development it seems to me both opaque and artificial to use the output of some environment-handling program, rather than simply a list of requirements that you can edit.

As for pre-commit hooks, I do not use them, except for maybe to pre-run flaks and black. As I understand it, poetry is a tool that enables you to set up in production exactly the code and configuration that was used in a particular release. The thing is, I am doing development, not production, and I work off the branch that is currently checked out in my repositories, not off releases.

WRT your latest comment, you seem to be talking about setting up a production-only version, something you download as-is, put on a beamline, and run. Which is fine for those who run experiments, but I need something that allows me to do development and mock mode tests, but still allows me to update dependencies in a simple manner to the most recent changes. So I will not interfere with the best way to make production releases, but I am asking for help in getting the things that I need, which are different. I do not know how you think I ought to be working. It is possible that there is a much smarter way of doing things, if only I had learned how. I would certainly look at good advice, but clearly it is easier for me to keep working in the way I already understand. than to try to figure out an alternative to fit in with poetry.

Short version: If we can get reliable and up-to-date requirements.txt files as checked in for mxcubeweb, mxcubecore, and mxcubeqt, I would love to use them.

fabcor-maxiv commented 1 month ago

We drifted quite a bit from the original topic. I hope it is okay that I close this ticket.

@marcus-oscarsson I realize I probably misinterpreted your comment that I was answering to in my previous message.

@rhfogh Feel free to open a dedicated discussion, ping me and I will make sure to follow-up. The short of it is that I believe every team (or even every contributor) has specific needs and constraints and it is very unlikely that we will ever manage to find and maintain a "development environment story" that fits all of them. We now have one way that is documented, it has its flaws (as demonstrated by https://github.com/mxcube/mxcubecore/issues/1061 for example) but maybe there is room for improvement in a manner that is not too specific to one particular team or contributor.

marcus-oscarsson commented 1 month ago

Ok, to close this :)