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

Yan 678 #30

Merged
merged 10 commits into from
Apr 27, 2021
Merged

Yan 678 #30

merged 10 commits into from
Apr 27, 2021

Conversation

davepallot
Copy link
Contributor

Hi Rod,
This branch is related to https://jira.skatelescope.org/browse/YAN-678. If you have time can you have a look and see it all checks out? You should be able to run the unit test independent of the others daliuge-engine/test/apps/test_plasma.py.

Cheers
Dave

@davepallot davepallot requested a review from rtobar April 27, 2021 02:28
@coveralls
Copy link

coveralls commented Apr 27, 2021

Coverage Status

Coverage decreased (-0.7%) to 76.737% when pulling e0f7487 on YAN-678 into ea54ebb on master.

Copy link
Contributor

@rtobar rtobar 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, other than some minor things. If I understand correctly, this is still following what we discussed last week, namely using plasma in a more "vanilla" way to demonstrate we can exchange data through it, and not necessarily trying to mimic/wrap what we are doing on the ingest pipeline + plasma_stman front.

daliuge-engine/dlg/drop.py Outdated Show resolved Hide resolved
daliuge-engine/dlg/drop.py Outdated Show resolved Hide resolved
@davepallot
Copy link
Contributor Author

Looks good, other than some minor things. If I understand correctly, this is still following what we discussed last week, namely using plasma in a more "vanilla" way to demonstrate we can exchange data through it, and not necessarily trying to mimic/wrap what we are doing on the ingest pipeline + plasma_stman front.

Thats correct, this is just a demonstration and the next sprint will likely be a 'replication' of the cbf in daliuge form (is my understanding).

@davepallot
Copy link
Contributor Author

@rtobar made changes: d0ee3fd

Copy link
Contributor

@rtobar rtobar left a comment

Choose a reason for hiding this comment

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

New small round of comments, sorry if I wasn't too clear the first time!

daliuge-engine/dlg/drop.py Outdated Show resolved Hide resolved
daliuge-engine/dlg/drop.py Outdated Show resolved Hide resolved
daliuge-engine/test/apps/test_plasma.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rtobar rtobar left a comment

Choose a reason for hiding this comment

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

Looks great now! Please go ahead with merging when convenient.

@davepallot davepallot merged commit a6d98c2 into master Apr 27, 2021
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

4 participants