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

Add threads-per-node option for vm-instance #290

Merged

Conversation

heyealex
Copy link
Contributor

This commits add the threads-per-core advanced machine features option to the vm-instance module. Default is 1, as it is typically more performant in classic HPC workloads.

Submission Checklist

  • Have you installed and run this change against pre-commit? pre-commit install
  • Are all tests passing? make tests
  • If applicable, have you written additional unit tests to cover this
    change?
  • Is unit test coverage still above 80%?
  • Have you updated any application documentation such as READMEs and user
    guides?
  • Have you followed the guidelines in our Contributing document?

@tpdownes
Copy link
Member

Please block until #283 is merged.

@tpdownes
Copy link
Member

tpdownes commented May 10, 2022

#283 has been merged. No longer blocked but there is a merge conflict @heyealex

@heyealex heyealex force-pushed the tf/vm-instance/threads-per-core branch from aa25698 to ab817bd Compare May 10, 2022 22:48
@heyealex
Copy link
Contributor Author

Conflict was addressed, git didn't handle adding two variables at once well. It should be ready for review @cboneti

Copy link
Member

@cboneti cboneti left a comment

Choose a reason for hiding this comment

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

We have seen multiple users confused by the fact that they pick a c2-standard-60 and can only find 30 cpus. With this new default, that would happen all the time, so I would add a note here describing the default behavior. Maybe also in the examples description when they use vm instances?

@cboneti cboneti assigned heyealex and unassigned cboneti May 10, 2022
This commits add the
[threads-per-core](https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_instance#threads_per_core)
advanced machine features option to the vm-instance module. Default is
1, as it is typically more performant in classic HPC workloads.
@heyealex heyealex force-pushed the tf/vm-instance/threads-per-core branch from ab817bd to 15f4d1d Compare May 10, 2022 23:38
@heyealex heyealex assigned cboneti and unassigned heyealex May 10, 2022
@cboneti cboneti assigned heyealex and unassigned cboneti May 11, 2022
@heyealex heyealex merged commit 8b2377f into GoogleCloudPlatform:develop May 11, 2022
@heyealex heyealex deleted the tf/vm-instance/threads-per-core branch May 11, 2022 15:45
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.

3 participants