[WIP] Perform pip -e install in configure step already.

Related to #279 (closed)

I propose to call pip install -e for a DUNE python module in the configuration step already, instead when building all. The editable install would be disabled when disabling the venv setup.

There is then only the target metadata_${envtargetname} remaining in all. Maybe this could also be removed when not using the venv. No target related to the venv setup is remaining in all any more.

@tkoch With this you would have no pip install -e and no metadata target any more when calling make all., and even no one left when disabling the venv. What do you think?

Current Status: We will incorporate these changes into the changes of !1148 (merged).

TODO:

• Replace the , separator in cmake_flags with something different by <SEP>.
• Incorporate the changes into !1148 (merged).

assigned to @tkoch

Nightly tests seem to work (one failed pipeline but that looks like a runner problem).

@samuel.burbulla could you try the github test scenario workflow if you haven't already done so.

Also I would like to make sure that changes in the Python sources are correctly picked up with this approach. For example: Can one add a new script or change a script? What about changes to one of the recompiled Python modules like _fem.so. That will of course require a make all but I would hope that no new pip install -e is needed. We should make sure.

How can I test only one different branch in dune-common?

I played around with adding python scripts and changing libraries, and everything worked out as expected in my opinion (of course, you need a make all).

How can I test only one different branch in dune-common?

What do you mean? You can add the branch name in the github workflow and it is combined with master of the other modules if the branch name doesn't exist there. On the dune-nightly-test git repo you can trigger a pipeline and provide the variable CI_BUILD_REF_NAME with the branch name you want to use (again main will be used if the branch name doesn't exist).

But I'm not sure this answers your question?

Thanks, if it uses the main/master as fallback, it is clear!

Just a remark: a nice benefit of this approach is that the requirements satisfied output during make all would disappear which I'm not quite sure how to do otherwise. The editable installation happens with --no-index so no packages are downloaded but the local wheels are checked and that is causing the verbose pip output we are seeing. Looking for packages there is used in the nightly tests to obtaining e.g. the dune.common package form its previously generated wheel when building dune-geometry. But that should still work with @samuel.burbulla changes - it would just not happen during make all.

Moving the generation of the metadata file as well should actually be possible as well as long as it happens after the dune package was taken care of.

This benefit was one of my main motivations to do this.

Moving the generation of the metadata would be nice, too. How could we achieve this?

Can we simply convert the add_custom_target for the metadata to dune_execute_process(COMMAND... as I did this for the pip call?

Simply replacing the metadata targets did not work out. Maybe I did something wrong. @andreas.dedner Do you have a specific idea in mind?

I managed to perform the generation of the metadata files in the configuration step.

Now, make remains only with

[ 20%] Built target _typeregistry
[100%] Built target dunecommon
[100%] Built target _common

I tested the internal dune-venv, an external venv and installing the packaged source distribution. Everything works as expected in my opinion.

@andreas.dedner Could you have a look and test?

Hi Samuel. Thanks for looking at this. Did you test the full nightly builds as well with branch?

Btw: our pypi packages seem broken at the moment due to some pip issue. https://github.com/adedner/dune-testpypi/actions/runs/2273111986 There is a pre release version of pip that is used that then leads to some version conflict with our >=21 versioning. A simple --pre install on my system locally now also fails. I have no idea why. If you have any idea how what the problem could be let me know Andreas

The nightly builds are working (except some minor issues with dune-alugrid unrelated to my changes). I also triggered a run on the dune-testpypi on this branch which was successful.

The pypi packages are working again with the changes on feature/generalize-packaging-script, adding a .a helps to include the pre-release.

@samuel.burbulla can you update the MR description about the metadata target?

Just ran the test scenario workflow on github and a few fail. Not sure why.

added 38 commits

• 4cbae614...103c05be - 36 commits from branch master
• c0a3521c - Perform pip -e install in configure step already.
• 5203e2ef - Remove one ALL.

Compare with previous version

I'm sorry I have very limited time at the moment :(. So I'm just going to throw this in here (probably the wrong place) but I see a third solution to the not "polluting" the all target issue. dunecontrol all could run both make all and make python_install_editable. Then dunecontroll all would do the same as before but there is also the possibility to build the targets separately.

All the targets for the metadata now have been gone, everything is done during configuration.

That means, nothing is polluting any more the all target. If you find some time, can you have a look and see if this already matches your needs?

configure leaves the virtual env in a "broken" state now, right? The Python packages are installed editable (i.e. symlinked) but are not functional? Conceptually it seems to be the wrong way around, "installing" first and then building.

But noone can expect anything to work after configure and I can see how the setup can be viewed as a configuration step especially since "install editable" is just linking and not really installing.

Yes, exactly! Everything works as before relying on the editable install.

added 21 commits

• 5203e2ef...c31f2b54 - 18 commits from branch master
• 3af2fab0 - Perform pip -e install in configure step already.
• a879b979 - Remove one ALL.
• 453c1d87 - Move generation of metadata file into configuration.

Compare with previous version

added 1 commit

• 70101aa2 - Add missing STATUS.

Compare with previous version

added 1 commit

• 9e6a67d9 - Escape quotes in cmake_flags.

Compare with previous version

added 1 commit

• d5c4e5fe - Add some escaping backslashes to correctly parse cmake_flags.

Compare with previous version

added 1 commit

• 79ea69de - Use delimiter "," to correctly parse cmake_flags.

Compare with previous version

added 72 commits

• 79ea69de...183ca5b6 - 71 commits from branch master
• b6941e0b - Perform pip -e install in configure step already.

Compare with previous version

Ready from my side. I will run the pipelines, GitHub testpypi and Nightly tests again.

@andreas.dedner When all of the pipelines succeed, could you have a final look?

Two remarks:

1. I had to remove the DEPENDS flag of the dune_python_install_package function as it can not be appended anymore. In my tests this seems not to be necessary and also simplifies the call of the function. I propose to deprecate the argument with a warning. Do you see any problems with that?

2. The list of cmake_flags had to be transformed into a string with "," as delimiter, because different platforms show differing behaviour now when the list is expanded. Sometimes I needed two escape characters, sometimes not... The function in packagemetadata.py has been adapted accordingly, is this fine or does this occur anywhere else?

If there are no objections, I will merge this in the next days.

@samuel.burbulla @santiago.ospina: the changes here are going to clash heavily with !1148 (merged). Perhaps it is possible to combine this directly in !1148 (merged) without merging this MR? @samuel.burbulla, have you had a look at Santiago's changes?

That makes sense. As far as I understand, changes here fit with the idea of installing any python package from this function. Maybe a recap for @samuel.burbulla: !1148 (merged) is a work in progress that reactors dune_python_install_package into small functions that can be combined to either install the bindings or any other python package from source. See !1148 (comment 116789) for details of the intended sub-functions.

Personally, either type of merging that you prefer is ok. In any case, the merge conflicts will have to be sorted out somewhere...

I agree somehow that it makes sense to combine those MRs. Unfortunately, I do not understand everything what's going on in !1148 (merged), so I'm not sure if I will merge this correctly. Therefore, my proposal is to rebase !1148 (merged) on this MR, what basically coincides with merging this MR first and rebasing !1148 (merged) on master.

General question: Isn't it preferable to have smaller MRs with clear objectives/changes and a working pipeline that are merged separately?

Regarding !1148 (merged), I will have a look, add comments and raise questions.

changed title from Perform pip -e install in configure step already. to [WIP] Perform pip -e install in configure step already.

changed the description

assigned to @samuel.burbulla and unassigned @tkoch

How does this integrate with the efforts in !1104 (closed)?

This will very likely lead to some serious merge conflicts.

that's clear but does it make it more difficult conceptually to realize !1104 (closed)?

As far as I can see there shouldn't be a problem as long as you just wrap the venv related calls by a USE_VENV. Everything (metadata etc.) is as before, it's just that the commands are now executed in the configure stage already.

changed the description

changed the description

Closed because changes are incorporated in !1148 (merged).

closed

mentioned in merge request !1148 (merged)