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

Eagle 869 2 #211

Merged
merged 10 commits into from
Nov 3, 2022
Merged

Eagle 869 2 #211

merged 10 commits into from
Nov 3, 2022

Conversation

james-strauss-uwa
Copy link
Collaborator

Added doxygen documentation for the 'persist' and 'streaming' component parameters to all Data drops. Both parameters are false by default. Except for the 'persist' parameter on File components, which is true by default.

Updated the Logical Graph JSON schema to reflect removal of 'streaming' and 'persist' attributes. They are now component parameters (elements in the 'fields' array) instead. Made some other changes to the schema were it had become out-of-date.

Pinned pyarrow version to 9.0 to prevent issues with recently released version 10. Rodrigo is happy for that change to make it to master.

Modified all existing graphs to remove 'persist' and 'streaming' attributes and move to component parameters where required.

  • Since 'persist' mostly defaults to false, and all the previous attributes were false anyway, the 'persist' parameter was not required anywhere except on "File" components, to override the default 'true' value.
  • The 'streaming' parameter defaults to false, so it only needed to be added to a few components were streaming was required.

Lastly, some of the reproducibility unit tests used the 'streaming' attribute, so I removed those references. Could you take a quick look at those changes @pritchardn

@coveralls
Copy link

coveralls commented Oct 28, 2022

Coverage Status

Coverage decreased (-0.01%) to 82.174% when pulling 3e4579a on eagle-869-2 into 5eade9c on master.

Copy link
Collaborator

@pritchardn pritchardn left a comment

Choose a reason for hiding this comment

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

The changes to the reproducibility tests look fine to me.
The changes to the graph files also look reasonable.

At some point, we should think about how we can notify users of changes to graph component formats - I have some components and graphs of my own I need to update, but if I were an external user, that would be difficult to work out without going through the source.

@james-strauss-uwa
Copy link
Collaborator Author

Thanks for reviewing.

I agree regarding graph format changes. It bugs me that there are graphs in many different old formats throughout this repository. I started work on a unit test to check that all graphs in the repo conform to the schema. At the moment, many graphs do not. At some point I'll update all the graphs.

At the moment, the best approach I have for updating old graphs is to load them into EAGLE and save them out again. I try to make EAGLE able to load old graph formats where possible. Not ideal though.

We plan to deploy a new version of EAGLE soon. One that includes the "auto-fix errors" feature, which can attempt to fix some common issues in graphs, including some issues related to out-of-date component definitions etc.

Copy link
Contributor

@juliancarrivick juliancarrivick left a comment

Choose a reason for hiding this comment

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

I notice that you've removed the streaming and persist flags from all the direct drop definitions in the graphs but only added a corresponding field entry in some cases (never for streaming). Is there any reason why this is the case? I guess they'll default to false in most cases (except DataDrops?) so it functionally makes no difference but am just wondering 🙂

@james-strauss-uwa james-strauss-uwa merged commit 74b83f6 into master Nov 3, 2022
@james-strauss-uwa james-strauss-uwa deleted the eagle-869-2 branch November 3, 2022 02:20
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.

None yet

5 participants