pinning migration cfep-9

CJ-Wright commented 5 years ago

This is based on the awesome work by @beckermr in https://github.com/conda-forge/conda-forge.github.io/issues/712 and discusses how a new pinning system coupled with automatic pinning migrations might work.

CJ-Wright commented 5 years ago

@beckermr please let me know if I failed to represent your vision properly, I'm happy to edit as needed.

CJ-Wright commented 5 years ago

@conda-forge/core I think most of the important conceptual details are here (if not implementation details) so I'd like to invite discussion on if this is a thing we want and how we might go about making this.

beckermr commented 5 years ago

Thanks @CJ-Wright for putting this up!

I am reading the comments and my proposal in the thread above to make sure it all still hangs together. Some of the details on how changes to the pinnings file are ordered have been lost and these details answer some of the questions here.

I am going to take a stab at v3 of a more detailed plan in this comment. We can translate to markdown later. This plan differs a bit from the thread above, but I think it is better. In particular, in this version, the pinnings files have a time ordered version that can be used to track state.

The migrations proceed as follows:

We make a PR on the migrations repo with a change to the global pinnings file, generating a new version.
Once the change is merged, the bot picks it up and starts going through the graph in topo order.
A migration PR is issued for a node only if
1. The node depends on the changed pinnings.
2. It has no dependencies that depend on the new pinnings and have not been migrated.
3. It is not currently being migrated by an earlier migration.
Process 3 continues until we determine that the migration is sufficiently complete and the change is merged into the global pinnings file.

Some comments:

We would need to associate a version independent of git hash with each migration that orders them in time and thus orders the pinning files in time. A pinnings file could be versioned simply with a comment inserted at the top of the YAML containing the timestamp that the change was merged or something. We could use seconds from the epoch!
For a given migration, we only use the changes up to and including the commit that has the relevant pinning updates, even if further updates are merged for future migrations. This forces a single global pinning file to be used for a given subgraph that is being migrated. This seems required for consistency.
We allow for a local pinnings file to be present in each feedstock that would override the global pinnings if the version is later. In other words, the more recent pinnings file is always preferred by smithy.
Smithy should use the most recent pinnings file when building the .ci_support files. This makes sure that rerendering the feedstock always produces the correct pinnings.
The migration bot would write the new pinnings file as the local one, rerender the repo and then issue the PR.
Migration changes have to be merged into the global pinnings file in the order they were made.

The rule of preferring the most recent pinnings file covers all of the relevant cases.

If a feedstock has no local pinnings, then that is fine.
If the file is present, but matches the global pinnings, this is also fine.
In the middle of a migration, a feedstock that is being migrated would have a local pinnings file that should be used ahead of the global one. The local version would be ahead of global in this case since the migration bot would add the new pinnings file as the local one.
After a migration has ended and the changes from the migration are merged to the global pinnings file, then the global file will always be at the same version as the local one, or at a later version.
- If some other migration comes through later, it would then overwrite the same local pinnings file with a version that is by definition later than the current global one.
- If a feedstock adds a dependency that has a pinned version, then the version that gets used is from the most recent pinnings file between the global one and the local one.
- Adding a dependence in the middle of a migration would cause the repo to be detected by the bot and a migration PR would be issued.

Thus we don't need to have conda-smithy cleanup any old local pinnings files, though it could if we want.

I don't really want to allow more than one change to the pinnings of a given node at a time, mostly because we cannot guarantee that later pinning migrations can be mixed with earlier ones (e.g., both could touch the same pins for some weird reason). We need to ensure that all nodes in the subgraph that share the same pinned dependence are uniformly migrated from one version to the next. However, we can have more than one migration in flight at a time simply because they could touch different parts of the graph.

scopatz commented 5 years ago

One thing we don't have in this disucssion (that I have seen) is a location for what migrations are curently active. I propose that this info is put into libcflib, with a REST API. That way conda-smithy and the auto-tick bot can query what is active at any given time.

beckermr commented 5 years ago

Agreed @scopatz. I'd prefer to have the bot check this, but have conda-smithy only deal with pinning version numbers.

beckermr commented 5 years ago

@CJ-Wright Can I submit a change to your PR (if you add me to your fork), or should I open a new one from my own fork with the text above?

CJ-Wright commented 5 years ago

@beckermr I've added you as a collab to my fork.

beckermr commented 5 years ago

OK I have moved a more detailed proposal into the markdown. Comment away!

tadeu commented 5 years ago

Hi! I'd just like to point out this current issue that seems slightly related to the changes here (because it's related to the way config YAML files are handles): https://github.com/conda-forge/conda-smithy/issues/1025

Perhaps a "fix" could already come together out of the changes here?

beckermr commented 5 years ago

Yes @tadeu. Right now the global conda-forge settings stomp on any local settings. This is probably for the best, given the purpose of global pins. However a discussion of these issues should be located in the linked issue so I will stop. ;p

CJ-Wright commented 5 years ago

Just to make sure we're on the same page. What do we want to happen when we have multiple migrations at once which may touch the same package (implementation considerations aside)?

My impression is that we have three options:

Halt the second migration PR on that feedstock until the first is merged/closed
Have the second supersede the first, with the second pinning file containing the first's data
Merge the two migrations in a way which causes multiple builds to occur with each combination

Are their more options, which one do we want?

beckermr commented 5 years ago

I say 1. 2 is acceptable to me but could involve the bot making commits to PRs that people are actively working on. 3 is overkill since we won't need both versions in most cases.

beckermr commented 5 years ago

Another proposal would be gut everything above and simply have the bot migrate any commit to the global pinnings file in topo order. This is a sort of "hog wild" approach, but it might not actually have that many issues associated with it. The key is to always produce consistent environments. If the pinning we adjust forces a specific version of a library to be used, then conda will take care of the rest.

scopatz commented 5 years ago

Wait, isn't (3) the right answer above? Like what if we had a compiler migration and an OpenSSL migration? Shouldn't those both happen independently?

scopatz commented 5 years ago

Maybe this is a fundemental question, but do pinning updates always/sometimes/never expand the build matrix?

beckermr commented 5 years ago

Hmmmmm. So I am excluding compiler migrations from this system. Nothing above introduces a new label on anaconda.org. Instead the system I outlined above simply puts the new packages in the main channel under the assumption that the shift in pinning produces packages with different requirements for the pinned item. In this case conda will generate a correct environment.

For migrations of critical infrastructure like compilers, I think we want separate labels and custom work.

beckermr commented 5 years ago

Whether or not a change in pinning increases the build matrix is a matter of taste. For a simple dep, like hdf5 or something, I don't see why we need both versions. As the packages get migrated in topo order, people's envs will update themselves properly once all of the requirements are satisfied.

scopatz commented 5 years ago

Nothing above introduces a new label on anaconda.org.

Ahh, ok I think I misunderstood the whole point of this exercise. If it isn't adding a label then (2) seems like the right approach

beckermr commented 5 years ago

This does raise an important question. If changes to the pinnings file don't generate packages conda can determine are incompatible, then none of the above will work. OTOH, if conda knows, then migrating in topo order is not really needed.

CJ-Wright commented 5 years ago

Migrating in topo order will matter either way, since if we migrate the bottom first it won't get the packages it needs. Currently this causes unsatisfiable dependencies errors, making the PR error on arrival which is kinda a waste of time. The other outcome is even worse, as we might not actually rebuild the package with the rebuild libraries meaning that we think we did something and ended up doing nothing but bumping the build number.

CJ-Wright commented 5 years ago

My main point with 2. was that if we update a pin, say openssl, then update it again right after then we shouldn't even bother with the first pin since it will be so short lived that not having packages for it might be ok. This would break things that need the first version of openssl but not the second, but that might be ok since we have collectively said the second version is the one we want to support long term.

beckermr commented 5 years ago

Good point about topo order and dependencies being available @CJ-Wright. I'm happy to go with 2 as well.

CJ-Wright commented 5 years ago

Based on this discussion I think the following workflow may work?

Propose a new pinning
Bot takes the new pinning file and applies it to the impacted feedstocks in topo order
If a new pinning is proposed send a second migration, if it impacts some of the feedstocks of the first migration then the maintainers can decide if they want to deal with both PRs or the most recent (if they just deal with the most recent then they can close the second one when the first one goes in and then that migration will also continue). (We can even try to make the bot smart about this, since it could put the Closes #42 at the beginning of the newest PR which would auto close the first PR causing the first migration to go forward). This also gives the maintainers the option of doing the potentially easier first PR first, causing only the second migration to stall.
Continue until criteria is met
Update global pinning so new packages get the new pinning.

scopatz commented 5 years ago

Also, (2) is the "current behaviour"

beckermr commented 5 years ago

Sounds great @CJ-Wright! I can take a pass updating the proposal later today to include all of the rationale and comments.

CJ-Wright commented 5 years ago

Discussion from the bot meeting:

Power users might be interested in using newer pins before a migration is finished. This could be handled by having two pining files, one which is in migration and one which is stable. Staged is built against stable, while packages can be built against either the stable (default) or migration (which is available via a cli)
Rebuild for all pin moves vs. only ABI incompatible moves
Have way to access the graph looking at which packages have which pins

ocefpaf commented 5 years ago

Thanks to https://github.com/conda-forge/conda-forge.github.io/pull/786 we can call for a vote here!

@conda-forge/core please vote on CFEP-09 by approving this PR if you are :+1: or adding a :-1: in the first comment if you disapprove.

ocefpaf commented 5 years ago

I can't request changes on my own PR, but I think that the two files in these PRs need to be merged before we begin voting.

Ah. I read only cfep-09.md :grimacing:

I don't think the language of the two proposals are compatible.

Is that something you are planning on doing?

beckermr commented 5 years ago

I think we ended up blocked on the technical requirements of what we to support and not support. This needs more discussion before voting.

CJ-Wright commented 5 years ago

@ocefpaf I can't, I'm on sabbatical while I'm working on my thesis. I think @mariusvniekerk's approach is working so I'd go with that. I think that approach should be merged into the main cfep-09.md document.

beckermr commented 5 years ago

The approach from @mariusvniekerk does not address conda selectors in the pinning files.

ocefpaf commented 5 years ago

@ocefpaf I can't, I'm on sabbatical while I'm working on my thesis.

Your new picture fooled me :stuck_out_tongue_closed_eyes: Get back to your thesis!

I think @mariusvniekerk's approach is working so I'd go with that. I think that approach should be merged into the main cfep-09.md document.

Yep. The core meeting today was discussing @mariusvniekerk approach and it seems to be a winner.

The approach from @mariusvniekerk does not address conda selectors in the pinning files.

OK. We need more discussion then. I'll let @mariusvniekerk answer that though.

mariusvniekerk commented 5 years ago

@beckermr the implementation in https://github.com/conda-forge/conda-smithy/pull/1072 does support conda selectors fully

beckermr commented 5 years ago

Ahhh great! The proposal above did not say how they would be supported which was the source of my comment. Let’s go ahead with this then.

beckermr commented 5 years ago

This pr leaves open the issue of how to propose a new migration so that the pinning bot sees it and also how to update the global pinnings if I am not mistaken.

We will need to work this out before merging.

I think something close to the suggestions above will work using the migration files that will be supported when the PR above is merged.

pkgw commented 5 years ago

I know that I'm jumping into the discussion very late here, but one thing that jumped out at me on reading the proposal is that the YAML format for specifying migrations seems a little awkwardly matched to what it's trying to express. If the goal is to "decompose all of our migrations into a small set of primitives", how about having the data structure be a list of those operations?

- type: upgrade_version
  package: mylib
  version: "1.2"
- type: downgrade_version
  package: myotherlib
  version: "1.0"

Maybe this is more-or-less equivalent to the current examples, but the way that the migration type is separated from the relevant package versions feels like something that's going to make it hard to express certain changes.

CJ-Wright commented 5 years ago

@pkgw I think that part of the point here is that the actual algebra (upgrade, explosion, contraction, downgrade, etc.) is defined by the versions and the current state of the pinnings. One could imagine a situation where one package sees a downgrade where another (which hasn't been updated in ages) sees an upgrade.

CJ-Wright commented 5 years ago

Implementations to merge:

CJ-Wright commented 5 years ago

I think that we can do this by hand for the moment. I'd be happy to start automating the process, but I think it might be best to have tighter control over the system in the beginning.

CJ-Wright commented 5 years ago

@conda-forge/core This PR falls under CFEP Approval policy, please vote and/or comment on this PR. This PR needs 60% (10 members) of core to vote yea to pass. To vote please leave Approve (yea) or Request Changes (nay) reviews. If you would like changes to the current language please leave a comment or push to this branch. This vote will end on Sept 6th.

isuruf commented 5 years ago

I don't understand the downgrade strategy. For example, we have a legitimate downgrade use-case in jpeg where we need to downgrade to 8 to switch the underlying implementation to libjpeg-turbo

beckermr commented 5 years ago

Downgrades were declared an anti pattern and as far as I know are not explicitly supported. I think maybe one could use the ordering variant for strings to force a downgrade?

beckermr commented 5 years ago

Someone would need to consult conda smithy to test this though

CJ-Wright commented 5 years ago

See https://github.com/conda-forge/conda-smithy/pull/1136 for downgrade test, which seems to work.

CJ-Wright commented 5 years ago

@conda-forge/core for those who haven't voted yet, please do so!

CJ-Wright commented 5 years ago

I can't approve via github this since I'm the author, but I approve this CFEP. This gives us the 10 votes needed.

conda-forge / cfep

pinning migration cfep-9 #13