LIU-417: Add GraphConfig to translator #296
Conversation
Reviewer's Guide by SourceryThis PR adds functionality to apply GraphConfig to the translator, allowing configuration values to be applied to construct nodes before graph processing. The implementation includes a new graph_config module with utility functions for handling graph configurations, modifications to the LG class to support config application, and comprehensive unit tests. Class diagram for LG class with GraphConfig changesclassDiagram
class LG {
-f
-ssid
-apply_config
+__init__(f, ssid=None, apply_config=True)
}
class GraphConfig {
+apply_active_configuration(logical_graph: dict) dict
+is_config_invalid(logical_graph: dict) bool
+get_key_idx_from_list(key: str, dictList: list) int
}
LG --> GraphConfig : uses
Class diagram for new graph_config moduleclassDiagram
class GraphConfig {
+apply_active_configuration(logical_graph: dict) dict
+is_config_invalid(logical_graph: dict) bool
+get_key_idx_from_list(key: str, dictList: list) int
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @myxie - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try: | ||
| activeConfigurationID = logical_graph[ACTIVE_CONFIG_KEY] | ||
| activeConfig = logical_graph[CONFIG_KEY][activeConfigurationID] | ||
| nodeDataArray = logical_graph[GRAPH_NODES] |
There was a problem hiding this comment.
issue (complexity): Consider converting list structures to dictionaries for node and field lookups.
The use of lists requiring linear search for ID lookups adds unnecessary complexity. Convert the list structures to dictionaries for O(1) lookup performance:
# Instead of:
nodeDataArray = logical_graph[GRAPH_NODES] # list of dicts
idx = get_key_idx_from_list(node_id, nodeDataArray)
node = nodeDataArray[idx]
# Use:
nodeDataArray = {node['id']: node for node in logical_graph[GRAPH_NODES]}
node = nodeDataArray[node_id]
# Similarly for fields:
fields = {field['id']: field for field in node['fields']}
field = fields[field_id]This eliminates the need for get_key_idx_from_list while maintaining all functionality. The main loop becomes cleaner:
nodeDataArray = {node['id']: node for node in logical_graph[GRAPH_NODES]}
for node_id, fields in activeConfig["nodes"].items():
if node_id not in nodeDataArray:
LOGGER.warning("%s present in activeConfig but not available in Logical Graph.")
continue
node = nodeDataArray[node_id]
node_fields = {f['id']: f for f in node['fields']}
for field_id, config_field in fields['fields'].items():
if field_id not in node_fields:
continue
field = node_fields[field_id]
prev_value = field["value"]
field["value"] = config_field["value"]
LOGGER.info("Updating: Node %s, Field %s, from %s to %s",
node['name'], field['name'], str(prev_value), str(field["value"]))There was a problem hiding this comment.
I think this is an important step for moving towards the new schema approach (see LIU-57), and something we can do even without modifying the schema.
However, in this case, we are trying to modify the logical_graph itself and so I think we transform it to a dictionary only to re-transform it back to an array. Definitely an approach to keep in mind moving forward to resolve all the searching we do across the different lists.
There was a problem hiding this comment.
You're absolutely right, @myxie. Converting the list to a dictionary for efficient lookups is a great step towards optimizing the current implementation, especially as we move towards the new schema approach. While it does involve an additional transformation step, the performance benefits during the lookup phase can be significant, particularly for larger graphs.
If the transformation back to a list is a concern, you might consider maintaining both structures temporarily during processing. This way, you can perform efficient lookups and updates using the dictionary, and then easily convert back to the list format if needed for compatibility with existing code or output requirements.
As you mentioned, this approach can be a valuable interim solution to reduce complexity and improve performance without requiring immediate schema changes. It's definitely worth considering as part of a broader refactoring strategy. Let me know if you need any further assistance or if there's anything else you'd like to discuss!
JIRA Ticket
https://icrar.atlassian.net/browse/LIU-417
Type
[ ] Bug fix[ ] DocumentationProblem/Issue
When the translator is sent a LogicalGraphTemplate with an activeGraphConfig, we need a way of applying that config to the graph.
Solution
We apply the GraphConfig prior to updating the logical graph constructs and extracting the input/output apps. This makes sure the value updates occur to the construct nodes in the graph, not the application nodes in the graph. This is because it is from the Construct nodes that we are getting the 'unrolling' parameters.
Checklist
[ ] Documentation addedSummary by Sourcery
Add support for applying GraphConfig to LogicalGraphTemplate in the translator, enabling configuration of graph constructs. Refactor the LG class to support configuration application during initialization and introduce unit tests to validate the new functionality.
New Features:
Enhancements:
Tests: