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 issue_tracker field to provider model #593

Merged
merged 6 commits into from
Jul 6, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Nov 17, 2020

This PR begins tracking changes to the specification beyond v1.0. We may consider maintaining this as a separate branch, or versioning our models in another way (see #609).

@ml-evs ml-evs requested a review from CasperWA November 17, 2020 11:40
@ml-evs ml-evs added models For issues related to the pydantic models directly schema Concerns the schema models labels Nov 17, 2020
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #593 (78eb2e2) into master (513d5b8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #593   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files          67       67           
  Lines        3752     3753    +1     
=======================================
+ Hits         3478     3479    +1     
  Misses        274      274           
Flag Coverage Δ
project 92.69% <100.00%> (+<0.01%) ⬆️
validator 92.69% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/server/config.py 91.20% <ø> (ø)
optimade/__init__.py 100.00% <100.00%> (ø)
optimade/models/optimade_json.py 95.78% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 513d5b8...78eb2e2. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/updates_wrt_specification branch from 6fa077b to 9922221 Compare November 20, 2020 00:38
@ml-evs
Copy link
Member Author

ml-evs commented Nov 20, 2020

I'm considering splitting this PR into "fixes" and "features" following the comments around maintaining two parallel releases of the specification. We should have a discussion about how we want to version our models in the future.

Is the API stable enough to be able to pluck the models out into a separate repository where they can be versioned as releases, or does this add too much overhead on what will hopefully be fairly static objects? [additionally two model versions could not be used simultaneously with this approach]

Should we explicitly add logic for versioning models in this package, e.g. extend OptimadeField to include some kind of "supported versions" attribute per field, so that different implementations can be validated (for example) with different model versions?

I'll raise an issue on this.

@ml-evs ml-evs force-pushed the ml-evs/updates_wrt_specification branch from 9922221 to b5cdeae Compare November 23, 2020 13:48
@ml-evs ml-evs changed the title Adding upstream changes to the specification Specification changes beyond v1.0 Nov 23, 2020
@ml-evs ml-evs added blocked For issues/PRs that are blocked by required changes/clarifications to the specification. needs discussion labels Nov 23, 2020
CasperWA
CasperWA previously approved these changes Nov 25, 2020
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay :)
However! We should probably change our "supported specification version" to v1.0.0~develop?

Indeed, this version splitting may pose some problems, as I believe you also point out?

@ml-evs
Copy link
Member Author

ml-evs commented Nov 25, 2020

Indeed, this version splitting may pose some problems, as I believe you also point out?

Yeah, I think this is blocked until we discuss #609 (and discuss spec versioning in the meeting today).

@ml-evs ml-evs requested a review from JPBergsma as a code owner June 1, 2021 14:41
@ml-evs
Copy link
Member Author

ml-evs commented Jul 6, 2021

As it looks like OPTIMADE 1.0.1 will not exist, we need to update our package to the imminent 1.1.0 and fix our descriptions. I think I have done this in a10e6bb

@ml-evs ml-evs removed the blocked For issues/PRs that are blocked by required changes/clarifications to the specification. label Jul 6, 2021
@ml-evs
Copy link
Member Author

ml-evs commented Jul 6, 2021

This also sets off a cascade of schema updates (that we already tried to pre-empt for v1.0.1...). We will need to make a PR with these schemas into the spec repo before v1.1.0 can be released.

@ml-evs ml-evs requested a review from CasperWA July 6, 2021 13:47
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know we hadn't included issue_tracker yet 😮

README.md Outdated Show resolved Hide resolved
optimade/models/optimade_json.py Show resolved Hide resolved
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final suggestion - also, is there not also a test config file to add issue_tracker to, or maybe it doesn't really matter...?

README.md Outdated Show resolved Hide resolved
ml-evs and others added 2 commits July 6, 2021 16:22
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@ml-evs ml-evs requested a review from CasperWA July 6, 2021 15:28
@ml-evs ml-evs changed the title Specification changes beyond v1.0 Add issue_tracker field to provider model Jul 6, 2021
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get in!

@CasperWA CasperWA merged commit 5e3e45e into master Jul 6, 2021
@CasperWA CasperWA deleted the ml-evs/updates_wrt_specification branch July 6, 2021 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
models For issues related to the pydantic models directly schema Concerns the schema models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incoming model update (new field: issue_tracker)
2 participants