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

Refactor NodeSpec to Hardware Spec #11

Merged
merged 4 commits into from
May 25, 2023
Merged

Refactor NodeSpec to Hardware Spec #11

merged 4 commits into from
May 25, 2023

Conversation

caldempsey
Copy link
Contributor

This PR deprecates the old NodeSpec type in favor of Hardware spec

@vercel
Copy link

vercel bot commented May 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 25, 2023 1:40pm

Copy link

@cebilon123 cebilon123 left a comment

Choose a reason for hiding this comment

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

anonymous

}
}

func getHardwareSpecFromInstanceTypesRespose(instance client.InstanceType, gpuMem, gpuCount int) types.HardwareSpec {

Choose a reason for hiding this comment

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

Shouldn't it be getHardwareSpecFromInstanceTypesResponse ? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not actually since we're pulling it from a InstanceType type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getHardwareSpecFromInstanceType is probably fine, thanks @cebilon123 for catching the typo 😄

@@ -99,7 +99,7 @@ type NodeTypesListResponse struct {
type ExecCreateParams struct {
Name string `json:"name,omitempty"`
Provider Provider `json:"provider"`
NodeTypeID string `json:"nodeTypeID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This will impact the CLI since it uses the same types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it so I probably bonked the 'select gpu' flag, will go through and test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -42,7 +42,7 @@ type Node interface {
// implemented at. For example, it could be implemented at a VM level for a bare-metal
// provider, at a container level, batch job level, etc. In each case, the node must
// serve as a host to run containers that are accessible via SSH.
InitNode(ctx context.Context, sshKey []types.SSHKey, nodeTypeID string, region *string) (node types.Node, err error)
InitNode(ctx context.Context, sshKey []types.SSHKey, hardwareSpec types.HardwareSpec, region *string) (node types.Node, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Simple naming is generally better. hardwareSpec -> spec

@@ -236,7 +232,8 @@ func (s *ExecService) Create(ctx context.Context, projectID string, params types
return nil, fmt.Errorf("failed to setup credentials: %w", err)
}

node, err := s.assignNode(ctx, params.NodeTypeID, params.Region, keys)
// Where a user assigns a session
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove internal comment

@@ -265,6 +262,7 @@ func (s *ExecService) Create(ctx context.Context, projectID string, params types
GitURL: params.GitURL,
}

// Where we tell Coreweave to assign an Exec/Session
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove internal comment

@noorvir noorvir merged commit dacfce7 into master May 25, 2023
2 checks passed
@noorvir noorvir deleted the callum/unw-160 branch May 25, 2023 13:58
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.

None yet

3 participants