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

Feature/extend batch support #285

Closed

Conversation

mccstan
Copy link

@mccstan mccstan commented Mar 7, 2024

Fix ignored configs when submitting jobs against Google Batch :
Doing :

  • Network config
  • Subnetwork
  • Private IP
  • Accelerator count
  • Accelerator type
  • cpu
  • ram
  • boot disk
  • data disk
  • machine type

@mccstan mccstan changed the title [WIP] Feature/extend batch support Feature/extend batch support Mar 11, 2024
Copy link
Contributor

@wnojopra wnojopra left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @mccstan !! A few points

  • First and most importantly, thank you for the PR. We very much appreciate community support.
  • Currently, the "source of truth" for dsub source code is in an internal repo. We won't be able to merge this PR directly. What we can do however is take it, add it to our internal system, and then acknowledge your contributions when it is shared back out to this repo.
  • We are looking into ways to update our contribution acceptance policy so that we can directly merge PRs from the community.
  • Could you please remove whitespace changes from this PR? I understand that several Python linters default to 4-space indents, but dsub's current linter uses 2. It may make sense to eventually adopt a 4-space indent, but when that happens we should make that change across all the files instead of just the few in this PR.

@mccstan
Copy link
Author

mccstan commented Mar 12, 2024

Thanks for reviewing @wnojopra

Feel free to integrate and share when it is possible.

I updated my linter to user 2 space style indent, but there is still some spaces remaining, I had to manually remove them.

Thanks.

@amircs
Copy link

amircs commented Mar 22, 2024

Great work, @mccstan! Hi @wnojopra, would it be possible for you to expedite the integration? I really appreciate your support!

@wnojopra
Copy link
Contributor

Hi @mccstan!
We'd love to incorporate your work into dsub and improve our ability to take in community contributions. We need to get our CONTRIBUTING doc updated, but in the meantime, can you please review and sign our Contributor License Agreement? You can send it to our IP team at [email protected].
Thanks!

@mccstan
Copy link
Author

mccstan commented Mar 28, 2024 via email

@nithinjoshy
Copy link

Hi,

Is there a timeline on when these features might be added and officially released?

Thanks!

@wnojopra
Copy link
Contributor

We've recently passed the IP contributors guidelines, and are now capable of accepting this PR into our internal repo. The next step is for me to run this through our tests and possibly add tests. Stay tuned, and thanks again to everyone for the interest and support.

@mccstan
Copy link
Author

mccstan commented Apr 12, 2024 via email

@wnojopra
Copy link
Contributor

Just a quick update on this - we have this PR working internally with tests. A release with this update is coming soon.

@wnojopra wnojopra mentioned this pull request May 6, 2024
@wnojopra
Copy link
Contributor

wnojopra commented May 6, 2024

Closing this PR as this has now been included with the latest release

Once again, thank you for the PR. We very much appreciate community support.

@wnojopra wnojopra closed this May 6, 2024
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.

4 participants