-
Notifications
You must be signed in to change notification settings - Fork 262
Support sycomp storage #4798
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
Support sycomp storage #4798
Conversation
Summary of ChangesHello @gqiu-sycomp-com, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Google Cluster Toolkit by introducing comprehensive support for the Sycomp Intelligent Data Storage Platform. It provides users with a suite of new blueprints and updated documentation, enabling seamless deployment, expansion, and integration of Sycomp Scale Storage clusters, including direct NFS access for SLURM environments, all within Google Cloud Platform. This addition aims to streamline the setup of high-performance storage solutions for HPC workloads. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces support for Sycomp Intelligent Data Storage Platform by adding four new example blueprints and associated documentation. The changes are well-structured and provide clear examples for deploying, expanding, and using Sycomp storage with a SLURM cluster. My review includes several suggestions to fix typos and improve formatting in the documentation and YAML comments for better clarity and correctness. I've also raised a concern about a documentation change that may refer to an unimplemented feature.
|
/gemini review |
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.
Code Review
This pull request introduces support for Sycomp storage by adding four example blueprints and associated documentation. The examples are well-structured, covering various deployment scenarios including initial setup, expansion, ECE configuration, and SLURM integration. My review focuses on enhancing the clarity and correctness of the documentation to improve the user experience. I have identified a typo and suggested improvements to make the instructions more clear and user-friendly.
sarthakag
left a comment
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.
Left some comments related to better naming of deployments. PTAL.
|
please assign to me once comments are addressed. Thanks! |
- Fix typo and grammer error - Rename deployments name to something more relevant to sycomp
|
/gemini review |
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.
Code Review
This pull request introduces support for Sycomp Intelligent Data Storage Platform by adding four example blueprints and associated documentation. The blueprints demonstrate how to deploy a new storage cluster, expand an existing one, use the Erasure Code Edition, and integrate with a SLURM cluster via NFS. The changes are well-structured and follow the project's conventions. My review includes one suggestion to improve the clarity of the documentation for a better user experience.
- Remove hardcode in the command
|
/gemini review |
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.
Code Review
This pull request introduces support for Sycomp Intelligent Data Storage Platform by adding four new blueprint examples and associated documentation. The changes are well-structured and provide clear examples for users. My review includes a few suggestions to improve correctness and clarity. Specifically, I've identified a file extension inconsistency in the documentation, two blueprints where a required name_prefix is missing which would likely cause deployment failure, and one blueprint with a hardcoded value that could be improved for better usability. These are straightforward fixes that will enhance the quality of the examples.
- Fix typo of file name - Add instructions for name_prefix - Set the name_prefix for slurm and ece example
|
/gemini review |
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.
Code Review
This pull request introduces support for Sycomp Intelligent Data Storage Platform by adding four new example blueprints. These blueprints demonstrate how to deploy a new Sycomp storage cluster, expand an existing one, use the Erasure Code Edition (ECE), and integrate it with a SLURM cluster. The implementation correctly sources the necessary Terraform modules directly from Sycomp's GitLab repository. The accompanying documentation is clear and helpful, though I've suggested a minor correction to a command example to prevent user confusion.
- Fix filename error in example command
|
/gemini review |
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.
Code Review
This pull request introduces support for the Sycomp Intelligent Data Storage Platform by adding four example blueprints and accompanying documentation. The blueprints cover deploying a standard storage cluster, an Erasure Code Edition (ECE) cluster, expanding an existing cluster, and integrating with a SLURM cluster using NFS. The examples are well-structured and follow existing patterns in the toolkit. The review identified two issues in the documentation: a minor discrepancy in the number of servers for the SLURM example, and a more significant issue where module parameter documentation is located in a private repository, hindering usability for the community. Once these documentation issues are addressed, this will be a valuable addition for Sycomp customers using the HPC Toolkit.
- Fix server number in README.md
|
/gemini review |
|
/gemini review |
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.
Code Review
This pull request introduces support for Sycomp Intelligent Data Storage Platform by adding four example blueprints and associated documentation. The blueprints cover basic deployment, deployment with Erasure Code Edition (ECE), cluster expansion, and integration with a SLURM cluster via NFS. The changes are well-structured, and the documentation is clear. The use of an external, version-pinned git repository for the Terraform modules is noted. My review found one area for improvement in the documentation to enhance user experience when expanding a cluster.
|
Hello, everybody, Could you please review the code I updated for me again? thank you! |
|
/gcbrun |
- use "use" in sycomp-storage-slurm instead of defining "network_storage" - Put the four blueprint examples related to sycomp into the exclude list of unittest
a000341
|
/gcbrun |
|
/gemini review |
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.
Code Review
This pull request introduces support for Sycomp Intelligent Data Storage Platform by adding four example blueprints and associated documentation. The changes are well-structured and the documentation is comprehensive. My review includes two suggestions for improvement: one to enhance clarity in the README to prevent user error, and another to improve the security posture of the sycomp-storage-slurm.yaml example by changing a default setting related to NFS security.
Provide 4 blueprint examples to create/expand Sycomp Scale Storage and access Sycomp Scale Storage by NFS.
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.