Add new option to CLI to run job, poll for job status and output of job #262
Conversation
📝 WalkthroughWalkthroughThe changes in the pull request focus on enhancing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant JobManager
participant Poller
User->>CLI: Run command with --runJobPoll
CLI->>JobManager: tango_runJob()
JobManager-->>CLI: Return job ID
CLI->>Poller: tango_runJobPoll(job ID)
Poller->>JobManager: Check job status
JobManager-->>Poller: Return job status
Poller->>CLI: Display job status
Poller->>JobManager: Check job status (repeat)
JobManager-->>Poller: Return job status
Poller->>CLI: Display job status
Poller->>JobManager: Check for completion or timeout
JobManager-->>Poller: Job completed or timed out
Poller->>CLI: Final job status
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
clients/tango-cli.py (1)
601-631: Add timeout or maximum retries to prevent infinite loopsThe infinite loop in
tango_runJobPollpolls the job status every 5 seconds. If the job never completes or an unexpected issue occurs, the loop could run indefinitely.Consider adding a timeout or maximum number of retries to prevent potential infinite loops:
max_retries = 120 # For example, retry for up to 10 minutes retry_count = 0 while retry_count < max_retries: ... time.sleep(5) retry_count += 1 else: print("Maximum retries reached. Exiting polling loop.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- clients/tango-cli.py (9 hunks)
🧰 Additional context used
🪛 Ruff
clients/tango-cli.py
17-17: Redefinition of unused
jsonfrom line 10Remove definition:
json(F811)
🔇 Additional comments (1)
clients/tango-cli.py (1)
76-78: Addition of--runJobPollargument is correctThe new command-line argument
--runJobPollhas been correctly added to the argument parser with appropriate help text. This enhances the CLI functionality as per the PR objectives.
| def tango_runJobPoll(): | ||
| end_string = "Job exited" | ||
| timeout_string = "Job timed out" | ||
| args.courselab = uuid.uuid4() |
There was a problem hiding this comment.
Convert UUID to string when assigning to args.courselab
At line 596, args.courselab is assigned a UUID object. Since args.courselab is used in URL construction and string formatting, assigning a UUID object directly may cause issues.
Convert the UUID to a string when assigning to args.courselab:
- args.courselab = uuid.uuid4()
+ args.courselab = str(uuid.uuid4())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args.courselab = uuid.uuid4() | |
| args.courselab = str(uuid.uuid4()) |
| if args.runJobPoll: | ||
| return response.text | ||
| else: | ||
| print(response.text) |
There was a problem hiding this comment.
Ensure consistent return statements in tango_jobs
Similar to tango_poll, the tango_jobs function conditionally returns response.text based on args.runJobPoll. This may lead to inconsistent behavior in the calling code.
Refactor the function to always return response.text:
def tango_jobs():
...
response = requests.get(...)
print(...)
- if args.runJobPoll:
- return response.text
- else:
- print(response.text)
+ return response.text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if args.runJobPoll: | |
| return response.text | |
| else: | |
| print(response.text) | |
| return response.text |
| @@ -12,6 +12,10 @@ | |||
| import argparse | |||
| import sys | |||
| import os | |||
| import time | |||
| from datetime import datetime | |||
| import json | |||
There was a problem hiding this comment.
Remove redundant import of json
The json module is already imported at line 10. Importing it again at line 17 is unnecessary and can be removed to avoid redundancy.
Apply this diff to remove the redundant import:
- import json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import json |
🧰 Tools
🪛 Ruff
17-17: Redefinition of unused
jsonfrom line 10Remove definition:
json(F811)
| if args.runJob: | ||
| return response.text |
There was a problem hiding this comment.
Ensure consistent return statements in tango_addJob
The function tango_addJob now returns response.text when args.runJob is set but does not return anything otherwise. This inconsistent return behavior may lead to issues if the calling code expects a return value.
Consider modifying the function to always return response.text for consistency:
def tango_addJob():
...
response = requests.post(...)
print(...)
- if args.runJob:
- return response.text
+ return response.text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if args.runJob: | |
| return response.text | |
| return response.text |
| if args.runJobPoll: | ||
| return response.text | ||
| else: | ||
| print(response.text) |
There was a problem hiding this comment.
Ensure consistent return statements in tango_poll
The function tango_poll conditionally returns response.text when args.runJobPoll is set and prints the response otherwise. This inconsistent behavior could cause issues in functions that depend on its return value.
Modify the function to always return response.text and handle printing in the calling code:
def tango_poll():
...
response = requests.get(...)
print(...)
- if args.runJobPoll:
- return response.text
- else:
- print(response.text)
+ return response.text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if args.runJobPoll: | |
| return response.text | |
| else: | |
| print(response.text) | |
| return response.text |
Add new CLI option runJobPoll to allow for easy lookup of a job throughout its lifecycle by first running the job, then checking both live and dead jobs for its inclusion, and then polling for output, concluding once the job has exited.
To test:
run
python clients/tango-cli.py -P {YOUR PORT} -k {YOUR KEY} --runJobPoll clients/job1/ --image autograding_imagesee result as follows:
