Skip to content

Add kGraphGRC message for setting/getting a YAML graph #553

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

Closed
wants to merge 2 commits into from

Conversation

iamsergio
Copy link
Contributor

Add kGraphGRC message for setting/getting a YAML graph

For #246

@iamsergio iamsergio changed the title Sergio/246 Add kGraphGRC message for setting/getting a YAML graph Apr 15, 2025
@iamsergio iamsergio marked this pull request as draft April 15, 2025 22:01
@iamsergio iamsergio requested a review from ivan-cukic April 15, 2025 22:01
@iamsergio
Copy link
Contributor Author

It's done, but will undraft only after #555 is merged, so I can rebase (change required for tests to work).

This allows Graph to use the yaml importer without having a cyclic
dependency between headers, and without moving stuff into .cpp files.

Part of task #246

Signed-off-by: Sergio Martins <[email protected]>
Part of task #246
Signed-off-by: Sergio Martins <[email protected]>
@iamsergio iamsergio marked this pull request as ready for review April 22, 2025 13:49
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
59.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Member

@wirew0rm wirew0rm 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 in general, just some questions on how this will interact with the lifecycle state of the graph and its blocks, see below. Might all be fine as it is and just need some more explanation for me to update my understanding of how we are going to use this.

@@ -343,6 +350,7 @@ class Graph : public gr::Block<Graph> {
propertyCallbacks[graph::property::kEmplaceEdge] = std::mem_fn(&Graph::propertyCallbackEmplaceEdge);
propertyCallbacks[graph::property::kRemoveEdge] = std::mem_fn(&Graph::propertyCallbackRemoveEdge);
propertyCallbacks[graph::property::kGraphInspect] = std::mem_fn(&Graph::propertyCallbackGraphInspect);
propertyCallbacks[graph::property::kGraphGRC] = std::mem_fn(&Graph::propertyCallbackGraphGRC);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like the semantics of the graph receiving a message and then replacing itself are a bit weird. For my mental model, the scheduler getting that message as the owner of the graph would make a lot more sense?
This might also be that my model of of the interplay between the components is a bit outdated and the graph has become a lot more dynamic lately.

} else if (message.cmd == message::Command::Set) {
const auto& data = message.data.value();
auto yamlContent = std::get<std::string>(data.at("value"s));
*this = gr::loadGrc(*_pluginLoader, yamlContent);
Copy link
Member

Choose a reason for hiding this comment

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

If I read this correctly, this will just replace the current graph with a new one which has been read from the yaml, right? It might do some cleanup in the old graph's destructor, but it cannot do blocking actions like correctly stopping the current graph, before removing it.

Question: Should it be/Is it possible to do this on a running graph, or does it have to be in stopped state? On the other hand this would make sending the message more complicated, since effectively you would need to trigger and wait for the state change before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While integrating in OD, I'm seeing a crash. I'll first add a unit-test for "Running Graph" case, so we capture the crash.

@ivan-cukic can comment on the rest

@iamsergio iamsergio marked this pull request as draft May 8, 2025 09:11
@iamsergio
Copy link
Contributor Author

being folded into #570

@iamsergio iamsergio closed this May 20, 2025
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.

2 participants