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.

Sign up for GitHub

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

Jump to bottom

LIU-308: Rename precious flag to persist #207

Merged
merged 3 commits into from
Oct 24, 2022
Merged

LIU-308: Rename precious flag to persist #207

merged 3 commits into from
Oct 24, 2022

Conversation

juliancarrivick
Copy link
Contributor

First commit is the function changes, second commit is non-functional: updating all the test graphs to be consistent with the new flag name.

@juliancarrivick juliancarrivick self-assigned this Oct 19, 2022
@coveralls
Copy link

coveralls commented Oct 19, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 82.186% when pulling af90a56 on liu-308-persist-flag into 42ab4fd on master.

juliancarrivick
juliancarrivick commented Oct 19, 2022
View reviewed changes
daliuge-engine/dlg/lifecycle/dlm.py
@@ -482,8 +483,8 @@ def handleCompletedDrop(self, uid):
return

drop = self._drops[uid]
if drop.precious and self.isReplicable(drop):
logger.debug("Replicating %r because it's precious", drop)
if drop.persist and self.isReplicable(drop):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't clear to me if this ticket covers changing the implementation of isReplicable() to be allowed by default? (probably should be in a separate PR anyway)

awicenec
awicenec requested changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@awicenec awicenec left a comment

Choose a reason for hiding this comment

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

These changes look good, but we need to make this change backwards compatible, since the associated ticket for EAGLE has not been implemented yet and even once it is, we can't be sure that people would use that version straight away. I guess we could catch the deprecated 'precious' flag in two places, either in the translator or in the graph_loader in the engine. Probably we should even do both. Essentially just replacing 'precious' with 'persist' at the earliest point possible in the two packages and leave the rest of the changes as they are now. In order to test that, we should keep one of the test graphs using 'precious'.

pritchardn
pritchardn requested changes Oct 19, 2022
View reviewed changes
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 so far look good to me 😄, backwards compatibility is probably wise.

There are a few additional files that may need this change too, and may need proper modification for backwards compatibility depending on whether EAGLE-869 ends up touching the following:

  • daliuge-translator
    • dlg/dropmake/lg.graph.schema - this is the json schema used to validate incoming lg graphs. If those coming from EAGLE with precious or persist should be valid.
  • docs
    • /development/app_development/eagle_app_integration.rst - mentions precious drops, should be changed to persisted too.
  • toos/xml2palette/xml2palette.py - Although this may end up being handled in EAGLE-869, this code takes the documentation strings for components and turns them into EAGLE palettes.

@juliancarrivick
Copy link
Contributor Author

A few questions in response:

  • is lg.graph.schema autogenerated from models or should I just edit that in place?
  • I think @awicenec mentioned at one point that Eagle has an overloaded definition of "precious", which I think is what is referred to in eagle_app_integration.rst. Is this right or have I got the wrong end of the stick?
  • Same for the precious reference in xml2palette.py

The backwards compatibility is a good point and it will be a good exercise for me to be adding some new tests too 🙂

@pritchardn
Copy link
Collaborator

A few questions in response:

* is `lg.graph.schema` autogenerated from models or should I just edit that in place?

I'm not 100% sure, but I think it can be edited in place.

* I think @awicenec mentioned at one point that Eagle has an overloaded definition of "precious", which I think is what is referred to in `eagle_app_integration.rst`. Is this right or have I got the wrong end of the stick?
* Same for the `precious` reference in `xml2palette.py`

I think you're absolutely right, my apologies 😅

@juliancarrivick
Copy link
Contributor Author

I only ended up adding the backwards compatibility in graph_loader for a couple of reasons:

  • I wanted to avoid duplicated logic
  • Further to this, there didn't seem to be a sensible spot to place any of that logic in the translator as that is purely dealing with graph traversal and any drop parameters just get passed through without a care for what they are.
  • Even if we added logic in there, that would only cover the translation of logical graphs to physical graphs, and we would still have to handle existing physical graphs that are using the precious flag

@juliancarrivick
LIU-308: Rename precious flag to persist
98bf385 
Ignore test graphs that don't need updating to pass tests as we'll do
this in a seperate commit.
@juliancarrivick
LIU-308: Update test graphs with persist flag
28bf425 
Ensuring not to update the overloaded precious flag in applicationArgs
or fields.
@juliancarrivick
LIU-308: Ensure drop precious flag is still handled
af90a56 
To ensure backwards compatibility rename it to persist when constructing
the drop instances. We also could add duplicated logic to do this during
translation from LG->PG, however we still need to handle this here as
old PGs might still have the flag. Additionally there don't seem to be
any obvious locations in the translator to add this logic as it is
exclusively dealing with graph traversal and just passes through any
properties is doesn't deal with straight through to the PG.
@pritchardn pritchardn self-requested a review October 24, 2022 04:04
pritchardn
pritchardn approved these changes Oct 24, 2022
View reviewed changes
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.

This looks good to me, happy to set a backwards compatibility test.

awicenec
awicenec approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@awicenec awicenec 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 now. Well spotted that the translator does actually not touch this particular parameter and indeed should not touch it, but just pass it on. That probably gave you also some impression about the mess in the translator code....

@juliancarrivick juliancarrivick merged commit e77aaa2 into master Oct 24, 2022
@juliancarrivick juliancarrivick deleted the liu-308-persist-flag branch October 24, 2022 05:53
awicenec pushed a commit that referenced this pull request Oct 10, 2024
@juliancarrivick
Merge pull request #207 from ICRAR/liu-308-persist-flag
00f74e1 
LIU-308: Rename precious flag to persist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pritchardn pritchardn pritchardn approved these changes

@awicenec awicenec awicenec approved these changes

Assignees

@juliancarrivick juliancarrivick

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juliancarrivick @coveralls @pritchardn @awicenec