-
Notifications
You must be signed in to change notification settings - Fork 12
Fix verify tests #281
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
base: main
Are you sure you want to change the base?
Fix verify tests #281
Conversation
PaliC
commented
May 13, 2025
- use workflows timeout
- remove magic number
- lint
- fix verify tests
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.
Pull Request Overview
This PR enhances test verification, replaces magic numbers with a constant timeout, and adds linting and logging improvements.
- Spread common settings into task configuration
- Refactored GitHub launcher to use
DEFAULT_GITHUB_TIMEOUT_MINUTES
and compute timeouts dynamically - Added detailed logging and updated verification cog and workflow YAML
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/discord-cluster-manager/utils.py | Added **common to merge shared config values into the task payload |
src/discord-cluster-manager/run_eval.py | Introduced logging, but left print statements for debugging |
src/discord-cluster-manager/launchers/github.py | Refactored GitHub launcher, added timeout constant, but broke a type hint |
src/discord-cluster-manager/consts.py | Defined DEFAULT_GITHUB_TIMEOUT_MINUTES constant |
src/discord-cluster-manager/cogs/verify_run_cog.py | Updated trigger_run to handle CUDA, added debug logs, but undefined lang |
.github/workflows/nvidia_workflow.yml | Installed jq to parse JSON payload in workflow |
Comments suppressed due to low confidence (2)
src/discord-cluster-manager/launchers/github.py:141
- The type annotation for
callback
is split across lines and will cause a SyntaxError. Combine it into one line or adjust indentation so the full signature is valid.
callback: Callable["[\"GitHubRun\"],
src/discord-cluster-manager/cogs/verify_run_cog.py:58
- The variable
lang
is not defined in this scope, which will raise a NameError. Ensurelang
is passed in or defined before this check.
elif lang == "cu":
print("sources are ") | ||
print(sources) |
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.
[nitpick] Replace this print statement with a logging call (e.g., logger.debug
) for consistent logging.
print("sources are ") | |
print(sources) | |
logger.info("sources are:") | |
logger.info(sources) |
Copilot uses AI. Check for mistakes.
print("sources are ") | ||
print(sources) | ||
logging.info(f"sources are {sources}") |
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.
[nitpick] Replace this print statement with a logging call (e.g., logger.debug
) for consistent logging.
print("sources are ") | |
print(sources) | |
logging.info(f"sources are {sources}") | |
logger.info(f"sources are {sources}") |
Copilot uses AI. Check for mistakes.
@@ -52,17 +52,21 @@ async def trigger_run(self, interaction: discord.Interaction, gpu: GPU, reporter | |||
sub_code = create_mock_attachment( | |||
"submission.py", Path("examples/identity_py/submission.py").read_text() | |||
) | |||
logger.info(f"sub_code: {sub_code}") |
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.
[nitpick] This debug logging statement may not be needed in production. Consider removing it or lowering its log level.
logger.info(f"sub_code: {sub_code}") | |
logger.debug(f"sub_code: {sub_code}") |
Copilot uses AI. Check for mistakes.