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

Require metadata on all topology #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Require metadata on all topology #26

wants to merge 1 commit into from

Conversation

henryh2
Copy link
Collaborator

@henryh2 henryh2 commented Nov 2, 2024

Requires metadata to be set on all topology, including both tree and block topology.

@henryh2
Copy link
Collaborator Author

henryh2 commented Nov 2, 2024

Two comments regarding this PR, the first of which is:

If the topology request payload specifies a topology plugin (either tree or block) that is different than the topology that can be provided, what should be the action? Return an error in the request?

@henryh2
Copy link
Collaborator Author

henryh2 commented Nov 2, 2024

And second, for MNNVL, is block topology always returned (current implementation), or should tree topology be returned in some situations?

@@ -26,6 +26,7 @@ import (
)

type Model struct {
Topology string `yaml:"topology"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need Topology field here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason is in the GetTestProvider() function in pkg/factory/provider.go. In the case of using a specific model file, which gets imported to file, that tree has to have a metadata tag so that the ToGraph() function can be called on line 82 (if it doesn't, one of the changes is that an error is thrown). So the "Topology" field is necessary in the model file so that the model can be correctly tagged in this context.

@dmitsh
Copy link
Collaborator

dmitsh commented Nov 2, 2024

If the topology request payload specifies a topology plugin (either tree or block) that is different than the topology that can be provided, what should be the action? Return an error in the request?

Toposim (and GTS) returns a collections of nodes with their local topology (a chain of switches and/or presence of nvlink).
From here we can construct either tree or block topology. So, IIUC, it is up to the requester to define. IMO we should consider tree topology as default.

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