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 JSON Lines specification appendix #531

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jul 19, 2024

This PR begins the work on #471, by defining the conventions on top of JSON Lines for deserializing an entire OPTIMADE API into a file.

@ml-evs ml-evs changed the title [WIP] OPTIMADE JSON Lines specification appendix OPTIMADE JSON Lines specification appendix Jul 29, 2024
Fix code block formatting

Update optimade.rst

Apply suggestions from code review

Apply suggestions from code review

Try to fix formatting again

Fix formatting and add note about provider fields
@ml-evs ml-evs force-pushed the ml-evs/jsonlines-draft branch from 6929f10 to c5f1f53 Compare July 29, 2024 20:51
@ml-evs ml-evs force-pushed the ml-evs/jsonlines-draft branch from 96b0231 to 78e158c Compare July 29, 2024 21:09
@ml-evs ml-evs marked this pull request as ready for review July 30, 2024 16:55
@ml-evs ml-evs requested a review from eimrek July 30, 2024 16:55
Copy link
Member

@eimrek eimrek left a comment

Choose a reason for hiding this comment

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

Thanks @ml-evs, and sorry for the delays. Now had some time to take a look. I like the proposal and it can be used for optimade-maker as well. But I wrote some comments for consideration.

Co-authored-by: Kristjan Eimre <eimrek@users.noreply.github.com>
ml-evs and others added 2 commits January 23, 2025 17:34
@ml-evs
Copy link
Member Author

ml-evs commented Jan 23, 2025

CI is blocked by #536, but perhaps you could take another look when you get a chance @eimrek? I'll invite some other reviewers too.

@ml-evs ml-evs requested review from eimrek, rartino and merkys January 23, 2025 18:04
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Looks good; and as we said during the web meet, very important extension.

However, how are relationships encoded? Maybe it would be nice if the example included a structure that has a relationship to a reference so one can see explicitly how that is encoded?

@ml-evs ml-evs requested a review from rartino February 3, 2025 13:59
rartino
rartino previously approved these changes Feb 3, 2025
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Looks good to me either. The part on "prefixes tied to the tools used to generate the file" is unclear to me, otherwise happy to merge this.

Co-authored-by: Andrius Merkys <andrius.merkys@gmail.com>
@eimrek
Copy link
Member

eimrek commented Feb 5, 2025

An additional question about custom prefixes, that is (still) not clear to me, is what should happen when provider1 creates a JSONL file, and provider2 subsequently serves this.

Should all _provider1 prefixes be replaced by _provider2?

or would _provider1 prefixes be retained and this would fall under 3.5.2?

Copy link
Member

@eimrek eimrek left a comment

Choose a reason for hiding this comment

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

Thanks a lot, Matthew, for working on this. Left some comments.

@ml-evs
Copy link
Member Author

ml-evs commented Feb 17, 2025

An additional question about custom prefixes, that is (still) not clear to me, is what should happen when provider1 creates a JSONL file, and provider2 subsequently serves this.

Should all _provider1 prefixes be replaced by _provider2?

or would _provider1 prefixes be retained and this would fall under 3.5.2?

I think "serving" is an entirely different thing; provider2 should decide whether to re-host the properties with their own prefix, or point to the provider1 definition if it is available. It's a bit tricky to write this out (also somewhat addressed by d2c095a) -- somehow there are properties that are going to be namespace scoped to just the file in question as the file is the "provider". They can then be served with any prefix as long as they are consistent.

@ml-evs ml-evs force-pushed the ml-evs/jsonlines-draft branch from e9ff554 to 2f9e2d7 Compare February 17, 2025 18:52
ml-evs and others added 3 commits February 25, 2025 14:12
Co-authored-by: Kristjan Eimre <eimrek@users.noreply.github.com>
Co-authored-by: Kristjan Eimre <eimrek@users.noreply.github.com>
@ml-evs ml-evs requested review from eimrek, merkys and rartino March 3, 2025 22:26
@ml-evs
Copy link
Member Author

ml-evs commented Mar 3, 2025

Thanks for the comments @eimrek, @merkys and @rartino -- I think I've clarified them now so this should be ready to go again.

Co-authored-by: Kristjan Eimre <eimrek@users.noreply.github.com>
@ml-evs ml-evs requested a review from eimrek March 4, 2025 14:29
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

Successfully merging this pull request may close these issues.

4 participants