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

Preview version of the OPTIMADE standard #403

Closed
JPBergsma opened this issue May 17, 2022 · 4 comments
Closed

Preview version of the OPTIMADE standard #403

JPBergsma opened this issue May 17, 2022 · 4 comments
Labels
type/proposal Proposal for addition/removal of features. May need broad discussion to reach consensus.

Comments

@JPBergsma
Copy link
Contributor

There are currently many PR’s on the OPTIMADE GitHub that are more or less finished, but have not yet been merged with the develop or the master branch. A number of examples of this are:

SMILES property #392
Adding the trajectories endpoint to OPTIMADE #377
Property definitions #376
Files endpoint #360
Allow boolean values in queries #348

For database providers who want to add some kind of functionality to their database that is not yet in the current master branch, it would be good to know what kind of ideas there are and how these will most likely be implemented in future versions of the OPTIMADE standard, so they do not need to reimplement this solution when the method in the standard differs from the method they chose.

Giovanni therefore thought that it would be a good idea to have a “preview” version of the OPTIMADE standard with the changes that are likely to end up in the final standard, even though they may still undergo some small changes. The approval requirements for such an addition would be lower than those of the master branch, but the PR should have been discussed, and written in an almost final form, ideally already with a proof of concept implementation.

In principle I think this could be a good idea. There are however still a few things that would need to be discussed.

  1. How many people should approve a change before it gets merged ?

For the main branch we now request that two persons accept the change.
Perhaps one would be enough for this preview version.

  1. Should all changes be included in this preview or only those that are backwards compatible?

In case there are changes that are not backwards compatible, the major version number, under which the OPTIMADE API with this particular change is served, should be increased. This can only be done once we know in which major release the change will be. In practice all the changes we have made so far are backwards compatible, and major releases will probably be separated by considerable amounts of time, so this may not be a big problem in practice.

  1. How should we implement this in the current GitHub environment?

If we were to merge the Pull request it would automatically get closed and a new PR should be made to continue discussing / editing the code, which would fragment the discussion.
This may not be a disadvantage though, some of the discussions become very long, so starting a new topic with a summary of what has already been discussed would make it easier for new people to join the discussions.

Another solution may be to have a separate repository in which separate pull request can be made to add the changes to the preview version.

  1. How are we going to version these preview additions?

It seems reasonable to allow that the changes are added to the main branch in a different order than they were added to the preview branch. The preview version of the OPTIMADE standard will probably be branched of the master branch so perhaps we could use the version number of the masterbranch on which it is based as a start and subsequently add an extra version number to indicate
The current OPTIMADE version is 1.1.0. we could then label the first preview version as 1.1.1-p.1
Here the patch version number has been updated to indicate that this release is newer than the 1.1.0 release.
if a second preview version would be made we could label this as 1.1.1-p.2 etc.
If a minor update is made to the OPTIMADE specification is released it's version would be come 1.2.0 the version number for the accompanying preview version would than become 1.2.1-p.2 indicating that it is still the same preview version but now based from a newer master version.

We could discuss this further at the OPTIMADE meeting at the end of May/ beginning of June.
I am curious to hear what your opinion about this is and how you would implement it.

@JPBergsma JPBergsma added the type/proposal Proposal for addition/removal of features. May need broad discussion to reach consensus. label May 17, 2022
@merkys
Copy link
Member

merkys commented May 30, 2022

This is an interesting proposal, which I believe could benefit the community of developers and adopters. Surely there are concerns to respond before we see this happen.

  1. How many people should approve a change before it gets merged ?

Do you mean approve as in approving an introducing PR? Well, then even one is quite much. The PRs you have listed rarely have even a single full approval. Can we somehow identify PRs nearing consensus and preview them?

  1. Should all changes be included in this preview or only those that are backwards compatible?

I think features breaking the backwards compatibility are less likely to be approved and integrated soon. Thus these should display higher level of approval before being included into feature preview.

Another question concerns conflicting features. There may be features that contradict one another, or affect the same portion of the specification.

  1. How should we implement this in the current GitHub environment?

We could have a preview branch and merge in not the PRs, but the branches associated with them. This surely would increase the manual work, but I do not see a solution which would not.

  1. How are we going to version these preview additions?

Why not append an ever-growing list of PR numbers? I.e., 1.1.1-pre.392.377.376.360.348 lists all the PRs from the list above.

@JPBergsma
Copy link
Contributor Author

Do you mean approve as in approving an introducing PR? Well, then even one is quite much. The PRs you have listed rarely have even a single full approval. Can we somehow identify PRs nearing consensus and preview them?

I indeed meant the number of persons required to approve a PR. I was wondering whether we could perhaps do something along the lines of. "If no one disagrees, I will add this to the preview version in two weeks from now."
That should give people enough time raise any objections.
I am not aware of a way to enforce this in Github though.

Good point about the conflicting PR's, Most PR's are orthogonal but there may indeed be cases where there is a conflict.
Ideally people in oldest PR would notify those that wrote the newer PR that there is a conflict. So it would not get merged without solving the conflict. But there is off course no guarantee this case would be handled correctly.

We could have a preview branch and merge in not the PRs, but the branches associated with them. This surely would increase the manual work, but I do not see a solution which would not.

Yes I think that could work.

Why not append an ever-growing list of PR numbers? I.e., 1.1.1-pre.392.377.376.360.348 lists all the PRs from the list above.

I think that could work, if no PRs are removed from preview version, without merging them in the main branch. Otherwise, it would become hard to see which preview version is newer.

@merkys
Copy link
Member

merkys commented Jun 2, 2022

I indeed meant the number of persons required to approve a PR. I was wondering whether we could perhaps do something along the lines of. "If no one disagrees, I will add this to the preview version in two weeks from now." That should give people enough time raise any objections. I am not aware of a way to enforce this in Github though.

We should take into account the added complexity of this suggestion with regard to added benefit. I am pretty sure I could get around without the preview version by simply stating, for example, "this is based on OPTIMADE v1.1.0 with #360 and #376 included".

Good point about the conflicting PR's, Most PR's are orthogonal but there may indeed be cases where there is a conflict. Ideally people in oldest PR would notify those that wrote the newer PR that there is a conflict. So it would not get merged without solving the conflict. But there is off course no guarantee this case would be handled correctly.

I would say the onus should be on the developers of the newer PR to discover and cure the conflicts.

Why not append an ever-growing list of PR numbers? I.e., 1.1.1-pre.392.377.376.360.348 lists all the PRs from the list above.

I think that could work, if no PRs are removed from preview version, without merging them in the main branch. Otherwise, it would become hard to see which preview version is newer.

Removing a PR from preview version sounds tricky. This should rarely be done.

@ml-evs
Copy link
Member

ml-evs commented Mar 25, 2024

I think lots of good points were raised here but somehow (nearly 2 years later) we've muddled through a lot of the big changes, so I will close this issue for now.

@ml-evs ml-evs closed this as completed Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal Proposal for addition/removal of features. May need broad discussion to reach consensus.
Projects
None yet
Development

No branches or pull requests

3 participants