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

OPTIMADE v1.2 release planning #429

Closed
ml-evs opened this issue Dec 5, 2022 · 14 comments · Fixed by #430, #485 or #518
Closed

OPTIMADE v1.2 release planning #429

ml-evs opened this issue Dec 5, 2022 · 14 comments · Fixed by #430, #485 or #518
Assignees
Labels
blocking-release This is a PR or issue that presently blocks the release of next version of the spec. priority/mid There is a consensus that this have average priority to sort out.
Milestone

Comments

@ml-evs
Copy link
Member

ml-evs commented Dec 5, 2022

Following from @merkys's comment on the mailing list, we should begin preparations for a v1.2 release.

I think I also foolishly "volunteered" to manage this release a while ago, but please help out if you want to. As part of this process, I've been doing a bit of issue triaging -- please re-open any issues that I closed that you think are still valid. In particular, I've also been clearing up the v1.2 milestone https://github.com/Materials-Consortia/OPTIMADE/milestone/3 which should now reflect what I write below.

We can use the comments here to have some async discussion that can perhaps be finalized at the final meeting this year.

Summary

As far as I see it, we need to decide:

  1. Whether to release v1.2 before the paper
  2. Whether to release and wait for providers to implement v1.2 before the paper
  3. Whether to write or merge any more PRs before releasing v1.2

My suggestion would be to finalize any PRs/issues we know we want in before the next meeting, then make a release candidate before Christmas. We can write the paper based on that release candidate and hopefully servers will start picking up the changes too (we can certainly get most of it into optimade-python-tools in January). We can submit the paper whenever with references to the release candidate, ready to be updated with the real release version in time for its (hopeful) publication.

Proposed changelog (updated 22/06)

I've just made a draft v1.2.0 release on GitHub (that everyone in the organization should be able to see) --- we could also convert this into a proper release candidate sooner rather than later v1.2.0-rc1, if desired. This draft has the following abridged (removed typo/testing PRs) changelog:

Outstanding PRs/issues

PRs that seem to required more time in the oven (and perhaps another round of in-person discussion):

Handled by future definition namespaces:

There are also some potentially blocking issues to address:

@ml-evs ml-evs added priority/mid There is a consensus that this have average priority to sort out. blocking-release This is a PR or issue that presently blocks the release of next version of the spec. labels Dec 5, 2022
@ml-evs ml-evs added this to the v1.2 milestone Dec 5, 2022
@ml-evs ml-evs self-assigned this Dec 5, 2022
@merkys
Copy link
Member

merkys commented Dec 6, 2022

Thanks for putting this together @ml-evs. Many of the features included in the current manuscript have been added post-v1.1, thus I think releasing v1.2 before publication would be a good idea.

#416 seems to be a real blocker. @rartino did a great job by summarizing the consensus, but it may take some time to convert that to a PR and merge it since a lot of new properties are proposed to be added.

#102 probably can be left out for this release (@blokhin, will you be able to support v1.2 with a per-database license file?)

#392 and #398 may be too far from reaching consensus at the moment. I am drafting an alternative proposal to implement #368, but I do not think it will be accepted any sooner.

@blokhin
Copy link
Member

blokhin commented Dec 6, 2022

@merkys per-database license file should be no problem! In general, I'd like to support the new release at the MPDS as early as possible.

@ml-evs
Copy link
Member Author

ml-evs commented Dec 6, 2022

The changes in #414 are actually somewhat breaking, as we are introducing a new mandatory info field license. Are we happy that this is introduced in a minor version release? (I vaguely remember a discussion of an implicit CC-BY if the license was not provided in this field)

@merkys
Copy link
Member

merkys commented Dec 7, 2022

The changes in #414 are actually somewhat breaking, as we are introducing a new mandatory info field license. Are we happy that this is introduced in a minor version release?

Oh, right. To stick to minor version we probably need to demote license to an optional field and define a default behavior ("check the website" as before, I assume?).

(I vaguely remember a discussion of an implicit CC-BY if the license was not provided in this field)

I think it is safer to say "do not assume anything if not given". This seems backwards-compatible to me 😃

@ml-evs
Copy link
Member Author

ml-evs commented Dec 7, 2022

Well, the property definitions PR definitely introduces many breaking changes to /info/<entry> too. We might need to discuss whether this is really already OPTIMADE 2 :)

@rartino
Copy link
Contributor

rartino commented Dec 7, 2022

Did we not agree to interpret SemVer for communication protocols to say that a backwards-breaking protocol change is one that breaks clients written to the older specification, but not one that requires change of the server code to conform to the newer version?

In that case, is adding a new mandatory field breaking? I suppose that, yes, very strictly speaking because we never explicitly say that a client MUST NOT crash on unrecognized non-database-specific fields. Still, I thought we agreed that this is just an omission we should just add as a clarification at some point.

I don't think the property definitions PR is backwards breaking (but I have not checked very carefully yet). In defining type in the info endpoint for OPTIMADE v1.1, I specifically insisted on inserting the following to make it possible to add this feature without being backwards-breaking: "For the purpose of compatibility with future versions of this specification, a client MUST accept values that are not string values specifying any of the OPTIMADE Data types, but MUST then also disregard the type field." I think that addresses the most clear place where the change could have been breaking.

@ml-evs
Copy link
Member Author

ml-evs commented Dec 9, 2022

Did we not agree to interpret SemVer for communication protocols to say that a backwards-breaking protocol change is one that breaks clients written to the older specification, but not one that requires change of the server code to conform to the newer version?

Sure (and we should write this up as I end up requesting to re-litigate this point every time...), but pragmatically, each of the providers will have a non-zero amount of work to do to support v1.2. I'm going through that process now for optimade-python-tools and could potentially have something ready for our next meeting on the 22nd.

In that case, is adding a new mandatory field breaking? I suppose that, yes, very strictly speaking because we never explicitly say that a client MUST NOT crash on unrecognized non-database-specific fields. Still, I thought we agreed that this is just an omission we should just add as a clarification at some point.

I don't think the property definitions PR is backwards breaking (but I have not checked very carefully yet). In defining type in the info endpoint for OPTIMADE v1.1, I specifically insisted on inserting the following to make it possible to add this feature without being backwards-breaking: "For the purpose of compatibility with future versions of this specification, a client MUST accept values that are not string values specifying any of the OPTIMADE Data types, but MUST then also disregard the type field." I think that addresses the most clear place where the change could have been breaking.

Thanks for pointing out that note, I hadn't spotted it. Maybe that digs us out of the hole on a technicality, but I'll still need to update the validator and client behaviour in optimade-python-tools to work with any new 1.2 databases. This does lead to a bit of an explosion of work in terms of supporting multiple v1 versions simultaneously, but perhaps that is unavoidable. As I said above, I'll try to present the work it required to support "v1.2" in optimade-python-tools and can make an informed decision then.

@rartino
Copy link
Contributor

rartino commented Dec 12, 2022

In that case, I don't think the key question is if the next version number is v1.2 or v2.0, but whether we need to go through the new features merged into develop and see if there are features that require so much work that we rather omit them from this release (saving them for the next release).

On the other hand, doesn't the new features mostly amount to optional features and a few new required fields? I get the point that to fully validate the info endpoint properties field is significant work (and lets then not talk about /trajectories if we merge that before the release...), but does the validator have to handle this before the release? Can't the validator just be changed to ignore the new mandatory fields under properties and type if it is unrecognized?

@ml-evs
Copy link
Member Author

ml-evs commented Dec 14, 2022

In that case, I don't think the key question is if the next version number is v1.2 or v2.0, but whether we need to go through the new features merged into develop and see if there are features that require so much work that we rather omit them from this release (saving them for the next release).

Agreed, in my comment I was also conflating the issue of who will actually have time to implement these features before we talk about them in the paper! We can see if anyone has any objections to the changeset outlined in the OP at the next meeting, and to vaguely cross-post my answer from there, I guess we are looking at January-February for a specification release now anyway.

but does the validator have to handle this before the release? Can't the validator just be changed to ignore the new mandatory fields under properties and type if it is unrecognized?

Yes, I'm fine with this, although the knock-on effect will be that the fuzzy query aspect of the validator will not be able to run -- assuming that providing properties in the old style is no longer supported (which seemed to be the case from last reading of the spec?).

@rartino
Copy link
Contributor

rartino commented Dec 15, 2022

but does the validator have to handle this before the release? Can't the validator just be changed to ignore the new mandatory fields under properties and type if it is unrecognized?

the knock-on effect will be that the fuzzy query aspect of the validator will not be able to run -- assuming that providing properties in the old style is no longer supported (which seemed to be the case from last reading of the spec?).

I'm not opposed that we add back the option for the old types to the specification alongside the "new" ones, with a "SHOULD"-level suggestion that one rather should use the new ones. It may be very helpful for people upgrading their code. Should we do that? I'd still mean to require the new mandatory fields, just that type is allowed to be a string of a single OPTIMADE type.

@ml-evs
Copy link
Member Author

ml-evs commented Jun 22, 2023

Suggestion is now to have release candidate 2 out by next week, at which point we can write the paper etc and hopefully find anything that breaks during implementations.

@ml-evs
Copy link
Member Author

ml-evs commented Jun 30, 2023

The consensus in today's meeting now seemed to shift back to delaying the release for several months until all of the PRs listed above are merged, and the paper will be written in such a way that assumes they will be merged, with the intermediate time being for initial implementations.

It's not entirely clear to me that this opinion is unanimous so please comment here if you disagree!

@rartino
Copy link
Contributor

rartino commented Apr 9, 2024

@ml-evs Should we really close this before we actually release v1.2.0 proper? It is (still) a pinned issue :)

@ml-evs
Copy link
Member Author

ml-evs commented Apr 9, 2024

Hmm, yes, this closed automatically...

@ml-evs ml-evs reopened this Apr 9, 2024
@ml-evs ml-evs unpinned this issue Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-release This is a PR or issue that presently blocks the release of next version of the spec. priority/mid There is a consensus that this have average priority to sort out.
Projects
None yet
4 participants