Fix test scheduling constraints#5383
Conversation
Summary of ChangesHello, 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 resolves a scheduling issue by removing an unnecessary temporal constraint. The constraint involved a test that lacked a corresponding build file, leading to a crash during the Terraform plan execution. Removing this constraint prevents the scheduling script from crashing. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a crash in the test scheduling script by removing a temporal constraint that referenced missing test build files. My review includes one suggestion to improve maintainability by adding a comment to explain the removal, in line with the project's documentation standards.
I am having trouble creating individual review comments. Click here to see my feedback.
tools/cloud-build/provision/list_tests.py (55-58)
To improve maintainability and provide context for future developers, it's better to leave a comment explaining why this constraint was removed, rather than just deleting it. This helps anyone looking at the code to understand the history without needing to check the git blame. This aligns with the style guide's emphasis on documentation and explaining the 'why'.
# Constraint for ("ml-a4-highgpu-slurm", "gke-a4") removed due to missing build files.References
- The repository style guide (line 34) states that in-code comments should explain the 'why' behind the code. Instead of silently removing this configuration, adding a comment explains the reason for the removal, which improves code maintainability. (link)
shubpal07
left a comment
There was a problem hiding this comment.
LGTM but we may need to make the check_resource_constraints function more defensive to future regressions.
9dabd02
into
GoogleCloudPlatform:develop
Summary
Removes the temporal constraint set (("ml-a4-highgpu-slurm", "gke-a4"), 2*60) from list_tests.py. The test ml-a4-highgpu-slurm was listed in the constraints but was missing a corresponding build file in the builds/ directory, which caused the scheduling script to crash with a KeyError during the Terraform plan.