EAGLE-1322: Stop EAGLE applying graph configuration before translation

james-strauss-uwa commented 2 weeks ago

At the moment, EAGLE applies the active graph configuration to the logical graph before translation. That is, it replaces values of node parameters with the values specified in the active graph configuration.

This PR removes that functionality, which will now be performed by the translator.

EAGLE now sends a slightly modified version of the LogicalGraph JSON to the DALiuGE translator. This version contains only the "active" graph configuration, or an empty dict, if no active graph configuration exists.

This PR also fixes a bug where cloning a LogicalGraph would NOT clone the state of the activeGraphConfiguration attribute.

Summary by Sourcery

Remove the functionality of applying the active graph configuration to the logical graph before translation, delegating this task to the translator. Update the LogicalGraph to send only the active graph configuration to the DALiuGE translator. Fix a bug related to cloning the LogicalGraph's activeGraphConfiguration state.

Bug Fixes:

Fix cloning of LogicalGraph to include the state of the activeGraphConfiguration attribute.

Enhancements:

Modify LogicalGraph to send only the active graph configuration or an empty dict to the DALiuGE translator.

sourcery-ai[bot] commented 2 weeks ago

Reviewer's Guide by Sourcery

This PR modifies how EAGLE handles graph configurations during translation by moving the responsibility of applying graph configurations from EAGLE to the DALiuGE translator. The implementation changes focus on modifying the JSON output format for translation and fixing a graph cloning issue.

Sequence diagram for LogicalGraph JSON preparation

sequenceDiagram
    participant EAGLE
    participant LogicalGraph
    participant DALiuGE Translator

    EAGLE->>LogicalGraph: Request JSON for translation
    LogicalGraph->>LogicalGraph: Check if active graph config exists
    alt Active graph config exists
        LogicalGraph->>LogicalGraph: Include active graph config in JSON
    else No active graph config
        LogicalGraph->>LogicalGraph: Include empty graph config in JSON
    end
    LogicalGraph->>EAGLE: Return JSON
    EAGLE->>DALiuGE Translator: Send JSON for translation

Updated class diagram for LogicalGraph

classDiagram
    class LogicalGraph {
        +toOJSJsonString(graph: LogicalGraph, forTranslation: boolean) : string
        +clone() : LogicalGraph
        -activeGraphConfig() : GraphConfig
    }
    class GraphConfig {
        +getId() : String
        +clone() : GraphConfig
        +toJson(config: GraphConfig) : Json
    }
    LogicalGraph --> GraphConfig : uses
    note for LogicalGraph "Now only includes active graph config in JSON for translation"

Updated class diagram for Translator

classDiagram
    class Translator {
        -eagle: Eagle
        +translate() : void
    }
    class LogicalGraph {
        +clone() : LogicalGraph
    }
    Translator --> LogicalGraph : uses
    note for Translator "Removed application of graph config to logical graph"

File-Level Changes

Change	Details	Files
Modified JSON output format for translation mode	Added conditional logic to only include active graph configuration when generating JSON for translation Added handling for cases where no active graph configuration exists Maintained full graph configurations output for non-translation use cases	`src/LogicalGraph.ts`
Removed premature graph configuration application	Removed code that applied graph configuration before translation Graph configuration application is now delegated to the translator	`src/Translator.ts`
Fixed graph cloning bug	Added code to properly clone the activeGraphConfig state when cloning a LogicalGraph	`src/LogicalGraph.ts`

Tips and commands

#### Interacting with Sourcery - **Trigger a new review:** Comment `@sourcery-ai review` on the pull request. - **Continue discussions:** Reply directly to Sourcery's review comments. - **Generate a GitHub issue from a review comment:** Ask Sourcery to create an issue from a review comment by replying to it. - **Generate a pull request title:** Write `@sourcery-ai` anywhere in the pull request title to generate a title at any time. - **Generate a pull request summary:** Write `@sourcery-ai summary` anywhere in the pull request body to generate a PR summary at any time. You can also use this command to specify where the summary should be inserted. #### Customizing Your Experience Access your [dashboard](https://app.sourcery.ai) to: - Enable or disable review features such as the Sourcery-generated pull request summary, the reviewer's guide, and others. - Change the review language. - Add, remove or edit custom review instructions. - Adjust other review settings. #### Getting Help - [Contact our support team](mailto:support@sourcery.ai) for questions or feedback. - Visit our [documentation](https://docs.sourcery.ai) for detailed guides and information. - Keep in touch with the Sourcery team by following us on [X/Twitter](https://x.com/SourceryAI), [LinkedIn](https://www.linkedin.com/company/sourcery-ai/) or [GitHub](https://github.com/sourcery-ai).

myxie commented 2 weeks ago

should probably coordinate the merge with Ryan when the translator side of the updates are ready

On this - don't worry too much about that; it can make things easier to test when EAGLE is a bit ahead of DALiuGE!

ICRAR / EAGLE