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

ENH: Add num_columns, num_rows to table spec #784

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

mferrera
Copy link
Collaborator

Resolves #768

@mferrera mferrera self-assigned this Sep 19, 2024
Comment on lines +19 to +20
path = Path(file_path)
meta_path = path.parent / f".{path.name}.yml"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forget why I type annotated this, and why it's in this PR, but let's call it a freebie

@mferrera
Copy link
Collaborator Author

I am worried about retrospective validation. Do these fields need to be optional for backward compatibility?

@tnatt
Copy link
Collaborator

tnatt commented Sep 19, 2024

I am worried about retrospective validation. Do these fields need to be optional for backward compatibility?

This is difficult.. how well is the sumo schema aligned with the komodo version? there is no versioning of the schema atm right?
I guess the safest is to have them optional for a transition period, in case users lock their komodo version?

@mferrera
Copy link
Collaborator Author

I think we just need to tackle versioning sooner rather than later. We have already made far too many changes and called it version 0.8.0. The more changes we implement the higher the risk we cause service interruptions for users. We need to get the fundamentals of deployments right otherwise we are going to build ourselves into a hole we can't get out of.

@perolavsvendsen
Copy link
Member

I think we just need to tackle versioning sooner rather than later. We have already made far too many changes and called it version 0.8.0. The more changes we implement the higher the risk we cause service interruptions for users. We need to get the fundamentals of deployments right otherwise we are going to build ourselves into a hole we can't get out of.

Yes.

(Missing) schema versioning has been a headache for years. We have discussed linking it to the fmu-dataio version, we have discussed how to migrate between schemas, etc etc. For now, the (not really a) solution has been to make sure we are backwards compatible at all times, and just stay on version 0.8.0. The story behind that is really just that the schema used on JS was on version 0.4 when development of fmu-dataio started. Then it was incremented a few times, until it sort of just froze on 0.8.0 🤷‍♂️

However, it needs to be extremely well aligned across fmu-dataio and Sumo and all consumers, including Webviz, REP, DynaGeo etc etc would have to also deal with it somehow, either explicitly or implicitly through some sort of abstraction. This also links a little bit to the discussions around products - today consumers will speak directly to the (meta)data via Elasticsearch.

So yeah, this is fun topic :)

@mferrera
Copy link
Collaborator Author

Updated these fields to be optional for now. They don't need to be, because the user input isn't required to populate them, but it will maintain backward compatibility.

Copy link
Collaborator

@tnatt tnatt left a comment

Choose a reason for hiding this comment

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

🚀

@mferrera mferrera merged commit 4787c91 into equinor:main Sep 23, 2024
13 checks passed
@mferrera mferrera deleted the add-table-row-and-columns branch September 23, 2024 09:41
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.

Add columns and rows count to data.spec for tables
3 participants