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

Add the full MSS test suite to the tests step of the build process #162

Closed
matrss opened this issue Dec 15, 2023 · 16 comments
Closed

Add the full MSS test suite to the tests step of the build process #162

matrss opened this issue Dec 15, 2023 · 16 comments

Comments

@matrss
Copy link

matrss commented Dec 15, 2023

The conda build should run the full MSS test suite as part of its build process, in addition to the existence and import checks that are already done.

This is to make sure that the dependency versions that are actually chosen in the conda build do not break any of the tests.

@ReimarBauer
Copy link
Contributor

We don't create by the build a binary with all used dependencies included. We don't know if at the next moment someone does an API change in theire package which brokes something on our software for someone installing MSS.

For finding such things we do regularly a scheduled test after the build. This finds changes in our stack which might need updates on our released version. We should improve this maybe on our scheduled test for other OS than linux and all relevant python versions.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

I might have misunderstood how conda-forge works.

My issue is this: right now if you install mss with mamba, you get pint=0.23 as a dependency. This breaks some tests, as you can see here: https://github.com/Open-MSS/MSS/actions/runs/7319432775/job/20050797319.

I thought that the package as distributed would only ever install the dependencies it was built with (and tested against here in the feedstock) and the pint update from 0.22 to 0.23 would require another build of the mss-feedstock to allow this dependency to be used (if the build succeeds). If that was the case, then we would only have to insert the MSS test suite in the recipes test step to automatically prevent mamba from installing a broken MSS.

Apparently this is not how it works though, since the latest mss-feedstock build is from before the release of pint=0.23: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=802266&view=logs&j=4f922444-fdfe-5dcf-b824-02f86439ef14&t=937c195f-508d-5135-dc9f-d4b5730df0f7

At which point in the build process can we insert the test suite to avoid mamba install mss from installing MSS with dependencies that break its test suite? Is that even possible?

@ReimarBauer
Copy link
Contributor

The build is only done at a release, not when a user installs it. Any library change which don't follow semantic versioning makes more trouble than those which did. But at all before a new major or minor of something is released we almost don't know if theire changes broke something in our software.

pint with its updates regularly causes problems. We should pin it to a limit on the next stable release and unpin it in development so that we can benefit from its improvements.

We make only a release when we have no failing tests. At this time also the feedstock build has no issues.

But the follow up problems were the reason we introduced the https://github.com/Open-MSS/MSS/actions/workflows/testing-scheduled.yml

We have to improve how our scheduled test informs us.

It is not so good to let it send only mails to me. First time a mail 3 weeks ago hit my account but I was distracted by other problems. Also sending it to slack would be an option. We have to find out.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

Does that mean that for conda-forge the package is built and tested with one set of dependency versions and then when a user installs the package they can receive an entirely different, untested, set of dependency versions? That is completely insane.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

By comparison, in nixpkgs a change to a dependency triggers a rebuild of all dependent packages (i.e. a change to pint would trigger a rebuild of MSS, and since derivations in nixpkgs should run the full test suite of the package they are building that would then fail).

I honestly thought conda-forge would do the same.

@ReimarBauer
Copy link
Contributor

ReimarBauer commented Dec 31, 2023

Does that mean that for conda-forge the package is built and tested with one set of dependency versions and then when a user installs the package they can receive an entirely different, untested, set of dependency versions? That is completely insane.

This is also a wanted feature. Because how else would a user get the most recent version of a dependency into the enviroment? if this brokes something it is our task to fix this ;)
It is not only the packaging, someone has also to improve the source ;)

@ReimarBauer
Copy link
Contributor

By comparison, in nixpkgs a change to a dependency triggers a rebuild of all dependent packages (i.e. a change to pint would trigger a rebuild of MSS, and since derivations in nixpkgs should run the full test suite of the package they are building that would then fail).

I honestly thought conda-forge would do the same.

We can of course do a pinning of everything but that makes a lot trouble too. e.g. how does a security fix of a dependent library reach a user?

@matrss
Copy link
Author

matrss commented Dec 31, 2023

Because how else would a user get the most recent version of a dependency into the enviroment?

In my opinion a package should be rebuild and tested for every dependency change and only when that succeeded should the published installable package use the new dependency. I.e. the pint release should have triggered a new build of MSS which builds and tests MSS with the latest set of dependencies and only if that succeeds should a users mamba install that package with those newer dependencies.

The version of a dependency that is used is part of the build of a package, so a new dependency version must always be a new build of the dependent package (because it is obviously running different code).

We can of course do a pinning of everything but that makes a lot trouble too. e.g. how does a security fix of a dependent library reach a user?

I think the pinning should be implicit for the published packages on conda-forge, i.e. everyone installing MSS should get the dependencies that were used for the latest successful build of that published package. Anything else would completely undermine the test steps done in the build process.

Then, when a new dependency version is published a rebuild can be automatically done and if it succeeds a new version of the package can be published, with a new set of implicitly pinned packages from its build. That is the only way to guarantee that a published package actually works (works as in: the existing tests don't fail) and is the model that is used in nixpkgs.

Security fixes would reach a user right after the new build succeeds and they update. If the new build fails then mamba should either use the latest successful build or refuse to install at all. As it is it silently installs a broken package without warning.

@ReimarBauer
Copy link
Contributor

You are right the new version of pint should have triggered me three weeks ago to file a bug to MSS and to create a new build for MSS with a pinnning of pint. Which I just do. But our code needs also been adapted to the new version of pint. For theses problems the build number gets increased.

We can discuss on MSS how this workflow can be automated.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

You are right the new version of pint should have triggered me three weeks ago to file a bug to MSS and to create a new build for MSS with a pinnning of pint.

The issue that mamba/conda silently installs a broken package still remains though. Even if this was fixed 3 weeks ago there would still have been a timeframe in which a mamba install mss would have installed a broken package. I see that as a huge flaw in how this packaging ecosystem works since mamba fails at its job: to install a package the same way it was built.

But our code needs also been adapted to the new version of pint.

I do not argue against that. But the default in a functioning package ecosystem shouldn't be: "I build a package with one set of dependencies, test that it works and then in the future install that same package with completely different dependencies and pray (not test) that it still works". conda-forge/mamba/conda fail at doing integration tests between packages, their dependencies and their dependents (by way of just running their test suites).

To phrase it differently: instead of silently installing a broken package mamba/conda-forge should either use known-good dependencies or refuse to install at all AND should shout at us to fix our code or pin pint to a lower version. As it is I am no longer surprised that I had mamba environments fail completely randomly for seemingly no reason in the past.

@ReimarBauer
Copy link
Contributor

You miss in your arguments that you can do a pinning in the stable branch. We decided not to do that but we can rethink if we want this.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

You miss in your arguments that you can do a pinning in the stable branch.

The same issue would remain for transitive dependencies, i.e. dependencies that we do not depend on explicitly but through others.

@ReimarBauer
Copy link
Contributor

There is no perfect solution but we try to improve us and to get closer.

The resolver is quite good for named transitive dependencies. I say named because some packages have some hidden by try except use an alternative which are not named in the recipe.

Also users have to learn not to use the mssenv for something else than mss. Mostly users are not developers and don't know about the idea of different envs and why that is so important.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

There is no perfect solution

There is nix ;) I am joking a bit, nix has its issues as well.

The resolver is quite good for named transitive dependencies.

Honestly, the issue is that there is a dependency resolver at install time at all. Dependency resolution should be a build-time only step. It is entirely unnecessary to resolve dependencies when installing and it leads to issues like this one where package installations become non-deterministic and break.

@ReimarBauer
Copy link
Contributor

Well for a developer it is quite clear that you need a new env for mss. For a user that is not. And also not for a developer when a new package should be added. So you would need to know what is not compatible to the wish and to remove it.

Because pixi works also on windows when it solves some of the identified problems maybe in the future worth to become our default. I am not sure if they have already the pinning solved but discussed this some month ago. The idea was to pin automatically to what is available on build time.

@matrss
Copy link
Author

matrss commented Dec 31, 2023

Environments are entirely orthogonal to the issue I am describing. You could create an env today, install MSS and get a working installation and do the same thing tomorrow and get a broken installation, with no changes to MSS in-between and no warnings to neither the user nor the package maintainers. I see that as a failure of the packaging system.

Environments of course have their own set of issues, and (I think) they would be completely unnecessary if packages didn't share their dependencies' installations (which they don't have to, that is just an architectural decision done in the ecosystem).

Pixi might be an improvement, I would have to see what that looks like to judge if it is.

AFAIU this issue is unsolvable right now, due to how the conda-forge ecosystem works, and I think we can close this issue.

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

No branches or pull requests

2 participants