-
Notifications
You must be signed in to change notification settings - Fork 37
feat: includeTopologies #254
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
base: main
Are you sure you want to change the base?
Conversation
This adds the ability to nest and compose topologies from others,
via existing topologies in the store or files on disk.
Ex:
```yml
...
includeTopooliges:
- /phenix/topologies/enterprise/phenix-configs/topolgy.yml
- /phenix/topolgies/ot/phenix-configs/topolgy.yml
- store-topo
nodes:
...
```
| if err := processIncludedTopologies(v1Spec); err != nil { | ||
| return nil, fmt.Errorf("processing included topologies: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure if doing this here is the right way to do it
| description: Array of topology file paths or store names to include | ||
| example: | ||
| - /phenix/topologies/base-topology.yml | ||
| - common-nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have to add this to the v2 spec, even though there is no v2 topology.
| } | ||
|
|
||
| return this.IncludeTopologiesF | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this function is useful. Should I add it to the interface (and return nil for v0 topology) ?
|
@glattercj @activeshadow let me know if you have thoughts. I am still looking at a few things and testing edge cases. |
|
Make sure to update the documentation with a minimal example 🤠 |
| example: [email protected] | ||
| Topology: | ||
| type: object | ||
| required: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to require one of nodes or includeTopologies?
|
This is not working out how I hoped. It helps a little but then we still have to maintain multiple different versions of the same app in different scenario files to correspond to different topos. I am thinking about a different way to tackle this through the API, allowing multiple topo/scenario pairs to get loaded at once and then "merged" at experiment creation time. apiVersion: phenix.sandia.gov/v0
kind: Workflow
metadata: {}
spec:
auto:
create: ${BRANCH_NAME}
configs:
- topology: ${BRANCH_NAME}-IT
scenario: ${BRANCH_NAME}-IT
- topology: ${BRANCH_NAME}-OT
scenario: ${BRANCH_NAME}-OT
...Then on exp creation:
I will leave this open for now. |
feat: includeTopologies
Description
This change introduces the ability to nest and compose topologies by leveraging existing stored topologies or defined in files on disk. This allows for modularity and reusability when defining complex topologies
Ex:
Related Issue
Tangential to #247
Type of Change
Please select the type of change your pull request introduces:
Checklist