Introduce non-blocking mode in subset command #963
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant CLI as Launchable CLI
participant Process as Subset Process
participant API as Launchable API
alt Non-Blocking Mode
CLI->>Process: Start Async Subset Request
Process->>API: Request Test Subset
Process-->>CLI: Immediate Return
else Blocking Mode
CLI->>API: Synchronous Subset Request
API-->>CLI: Return Test Subset
end
Poem
✨ Finishing Touches
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: 2
🧹 Nitpick comments (1)
launchable/commands/subset.py (1)
521-527: Consider enhancing non-blocking mode with status tracking.The current implementation could benefit from additional features to make it more practical:
- Add a command to list ongoing non-blocking subset requests
- Add a command to check the status of a specific request
- Implement a simple status tracking mechanism using the metadata directory
Example implementation approach:
- Store request metadata (timestamp, session_id, process_id) in a JSON file
- Add
subset list-requestscommand to show ongoing requests- Add
subset request-status <request_id>command to check specific requests- Implement automatic cleanup of old request metadata
Would you like me to provide a detailed implementation proposal for these enhancements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
launchable/commands/subset.py(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python package
launchable/commands/subset.py
[error] 637-637: "dict" is not subscriptable, use "typing.Dict" instead
🔇 Additional comments (2)
launchable/commands/subset.py (2)
135-143: LGTM! Well-documented CLI option.The
--non-blockingoption is implemented consistently with other options, and the help text clearly explains its purpose and limitations.
237-249: LGTM! Proper validation of non-blocking mode.The code correctly validates that
--non-blockingis only used with--observationmode, with appropriate error handling and user feedback.
launchable/commands/subset.py
Outdated
| def subset_request(client: LaunchableClient, timeout: Tuple[int, int], payload: dict[str, Any]): | ||
| return client.request("post", "subset", timeout=timeout, payload=payload, compress=True) |
There was a problem hiding this comment.
Fix type annotation to use typing.Dict.
The pipeline failure indicates that dict is not subscriptable. Use typing.Dict instead.
Apply this fix:
-def subset_request(client: LaunchableClient, timeout: Tuple[int, int], payload: dict[str, Any]):
+def subset_request(client: LaunchableClient, timeout: Tuple[int, int], payload: Dict[str, Any]):
return client.request("post", "subset", timeout=timeout, payload=payload, compress=True)📝 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.
| def subset_request(client: LaunchableClient, timeout: Tuple[int, int], payload: dict[str, Any]): | |
| return client.request("post", "subset", timeout=timeout, payload=payload, compress=True) | |
| def subset_request(client: LaunchableClient, timeout: Tuple[int, int], payload: Dict[str, Any]): | |
| return client.request("post", "subset", timeout=timeout, payload=payload, compress=True) |
🧰 Tools
🪛 GitHub Actions: Python package
[error] 637-637: "dict" is not subscriptable, use "typing.Dict" instead
| if is_non_blocking: | ||
| # Create a new process for requesting a subset. | ||
| process = Process(target=subset_request, args=(client, timeout, payload)) | ||
| process.start() | ||
| click.echo("The subset was requested in non-blocking mode.") | ||
| return | ||
|
|
There was a problem hiding this comment.
Add process error handling and cleanup.
The current implementation of non-blocking mode has several potential issues:
- No error handling for process creation/start failures
- No cleanup of zombie processes
- No way to track the status of the subset request
Consider implementing the following improvements:
if is_non_blocking:
+ try:
# Create a new process for requesting a subset.
- process = Process(target=subset_request, args=(client, timeout, payload))
- process.start()
+ process = Process(target=subset_request, args=(client, timeout, payload), daemon=True)
+ process.start()
+ # Store the process ID for potential future use
+ with open(os.path.join(app.get_metadata_dir(), f"subset_process_{session_id}.pid"), "w") as f:
+ f.write(str(process.pid))
+ except Exception as e:
+ msg = f"Failed to start non-blocking subset request: {str(e)}"
+ click.echo(click.style(msg, fg="red"), err=True)
+ tracking_client.send_error_event(
+ event_name=Tracking.ErrorEvent.INTERNAL_CLI_ERROR,
+ stack_trace=str(e),
+ )
+ sys.exit(1)
click.echo("The subset was requested in non-blocking mode.")
returnCommittable suggestion skipped: line range outside the PR's diff.
Konboi
left a comment
Konboi
left a comment
There was a problem hiding this comment.
I asked a question about the specifications on the project page.
|
I fixed code based on your review. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
launchable/commands/subset.py (2)
636-637: Add function documentation.Add a docstring to explain the function's purpose, parameters, and return value.
Apply this diff to add documentation:
def subset_request(client: LaunchableClient, timeout: Tuple[int, int], payload: Dict[str, Any]): + """Send a subset request to the Launchable API. + + Args: + client: LaunchableClient instance for making API requests + timeout: Tuple of (connect timeout, read timeout) in seconds + payload: Request payload containing test paths and configuration + + Returns: + Response from the subset API endpoint + + Raises: + requests.exceptions.RequestException: If the API request fails + """ return client.request("post", "subset", timeout=timeout, payload=payload, compress=True)
519-526: Consider implementing a request management system.The current non-blocking implementation could benefit from a more robust architecture:
Request Queue Management:
- Implement a queue system to manage multiple concurrent requests
- Add rate limiting to prevent resource exhaustion
- Provide a way to list and cancel pending requests
Status Monitoring:
- Add a new command to check the status of non-blocking requests
- Implement a simple status API endpoint
- Store request metadata in a structured format
Would you like me to propose a detailed design for these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
launchable/commands/subset.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (ubuntu-20.04, 3.12)
- GitHub Check: build (ubuntu-20.04, 3.11)
- GitHub Check: build (ubuntu-20.04, 3.10)
- GitHub Check: build (ubuntu-20.04, 3.9)
- GitHub Check: build (ubuntu-20.04, 3.8)
- GitHub Check: build (ubuntu-20.04, 3.7)
- GitHub Check: build (ubuntu-20.04, 3.6)
🔇 Additional comments (3)
launchable/commands/subset.py (3)
135-141: LGTM! Option definition follows best practices.The
--non-blockingoption is correctly defined as a hidden flag, following the reviewer's suggestion to mark it as experimental.
235-247: LGTM! Validation logic is thorough and consistent.The validation ensures that
--non-blockingis used only with--observationmode, with proper error handling that follows the codebase's patterns.
519-526:⚠️ Potential issueAdd robust process management and error handling.
The current implementation of non-blocking mode needs improvement in several areas:
- No error handling for process creation/execution
- No cleanup of zombie processes
- No way to track the subset request status
Apply this diff to add proper error handling and process management:
if is_non_blocking: + process = None + try: # Create a new process for requesting a subset. - process = Process(target=subset_request, args=(client, timeout, payload)) - process.start() + process = Process(target=subset_request, args=(client, timeout, payload), daemon=True) + process.start() + # Store process information for tracking + process_info = { + "pid": process.pid, + "session_id": session_id, + "timestamp": datetime.now().isoformat() + } + tracking_file = os.path.join(app.get_metadata_dir(), f"subset_process_{session_id}.json") + with open(tracking_file, "w") as f: + json.dump(process_info, f) click.echo("The subset was requested in non-blocking mode.") + click.echo(f"Process ID: {process.pid}") + click.echo(f"Track status: {tracking_file}") self.output_handler(self.test_paths, []) return + except Exception as e: + if process and process.is_alive(): + process.terminate() + msg = f"Failed to start non-blocking subset request: {str(e)}" + click.echo(click.style(msg, fg="red"), err=True) + tracking_client.send_error_event( + event_name=Tracking.ErrorEvent.INTERNAL_CLI_ERROR, + stack_trace=str(e), + ) + sys.exit(1)Likely invalid or redundant comment.
launchable/commands/subset.py
Outdated
| # Create a new process for requesting a subset. | ||
| process = Process(target=subset_request, args=(client, timeout, payload)) | ||
| process.start() | ||
| click.echo("The subset was requested in non-blocking mode.") |
There was a problem hiding this comment.
As far as I remember, click.echo outputs as stdout. It means this message ("The subset was requested in non-blocking mode.") will be included as subset out. So you should print this message as stderr. ( I know it's a little tricky 😅)
There was a problem hiding this comment.
Oh, thank you for telling me that.
There was a problem hiding this comment.
Could you run test with --non-blocking option once as test before merging this PR?
https://github.com/launchableinc/cli/blob/main/.github/workflows/python-package.yml#L102
or add example to this repo https://github.com/launchableinc/examples
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
launchable/commands/subset.py (1)
Line range hint
509-627: Add retry mechanism for subset requests.Consider adding a retry mechanism for subset requests to handle transient network issues, especially important in non-blocking mode where the user won't see immediate failures.
I can help implement a retry mechanism with exponential backoff if you'd like. Would you like me to open an issue for this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
launchable/commands/subset.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (ubuntu-20.04, 3.12)
- GitHub Check: build (ubuntu-20.04, 3.11)
- GitHub Check: build (ubuntu-20.04, 3.10)
- GitHub Check: build (ubuntu-20.04, 3.9)
- GitHub Check: build (ubuntu-20.04, 3.8)
- GitHub Check: build (ubuntu-20.04, 3.7)
- GitHub Check: build (ubuntu-20.04, 3.6)
🔇 Additional comments (4)
launchable/commands/subset.py (4)
6-6: LGTM!The
Processimport frommultiprocessingis correctly added for implementing the non-blocking mode.
135-141: LGTM!The
--non-blockingoption is well-defined with:
- Clear help text explaining its purpose
- Hidden flag as suggested in past reviews
- Proper type as a boolean flag
626-627: LGTM!The
subset_requestfunction is well-defined with:
- Clear type hints
- Proper parameter types
- Single responsibility
509-516:⚠️ Potential issueImprove process error handling and cleanup.
The current implementation of non-blocking mode lacks:
- Error handling for process creation/start failures
- Cleanup of zombie processes
- Process status tracking
Apply this diff to add error handling and process management:
if is_non_blocking: + try: # Create a new process for requesting a subset. - process = Process(target=subset_request, args=(client, timeout, payload)) - process.start() + process = Process(target=subset_request, args=(client, timeout, payload), daemon=True) + process.start() + # Store the process ID for potential future use + process_file = os.path.join(app.get_metadata_dir(), f"subset_process_{session_id}.pid") + with open(process_file, "w") as f: + f.write(str(process.pid)) + except Exception as e: + msg = f"Failed to start non-blocking subset request: {str(e)}" + click.echo(click.style(msg, fg="red"), err=True) + tracking_client.send_error_event( + event_name=Tracking.ErrorEvent.INTERNAL_CLI_ERROR, + stack_trace=str(e), + ) + sys.exit(1) click.echo("The subset was requested in non-blocking mode.", err=True) self.output_handler(self.test_paths, []) returnLikely invalid or redundant comment.
launchable/commands/subset.py
Outdated
| if is_non_blocking: | ||
| click.echo("Observation mode is automatically enabled in non-blocking mode.", err=True) | ||
| is_observation = True | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to ensure non-blocking mode is used only with observation mode.
The current implementation automatically enables observation mode when non-blocking is used, but it should validate that non-blocking mode cannot be used without observation mode.
Apply this diff to add the validation:
if is_non_blocking:
+ if not is_observation:
+ msg = "Non-blocking mode can only be used with observation mode"
+ click.echo(click.style(msg, fg="red"), err=True)
+ tracking_client.send_error_event(
+ event_name=Tracking.ErrorEvent.INTERNAL_CLI_ERROR,
+ stack_trace=msg,
+ )
+ sys.exit(1)
click.echo("Observation mode is automatically enabled in non-blocking mode.", err=True)
is_observation = True📝 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 is_non_blocking: | |
| click.echo("Observation mode is automatically enabled in non-blocking mode.", err=True) | |
| is_observation = True | |
| if is_non_blocking: | |
| if not is_observation: | |
| msg = "Non-blocking mode can only be used with observation mode" | |
| click.echo(click.style(msg, fg="red"), err=True) | |
| tracking_client.send_error_event( | |
| event_name=Tracking.ErrorEvent.INTERNAL_CLI_ERROR, | |
| stack_trace=msg, | |
| ) | |
| sys.exit(1) | |
| click.echo("Observation mode is automatically enabled in non-blocking mode.", err=True) | |
| is_observation = True |
Nice catch! But I think this validation (296aaf0) is really helpful not only user but also Launchable side... If we did, we can validate it correctly, right? WDYT?? |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
launchable/commands/subset.py (1)
303-330: Simplify nested if statements.The validation logic is correct, but the code structure can be simplified.
- if is_non_blocking: - if (not is_observation) and session_id: + if is_non_blocking and (not is_observation) and session_id:🧰 Tools
🪛 Ruff (0.8.2)
303-304: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
launchable/commands/subset.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
launchable/commands/subset.py
303-304: Use a single if statement instead of nested if statements
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (ubuntu-20.04, 3.12)
- GitHub Check: build (ubuntu-20.04, 3.11)
- GitHub Check: build (ubuntu-20.04, 3.10)
- GitHub Check: build (ubuntu-20.04, 3.9)
- GitHub Check: build (ubuntu-20.04, 3.8)
- GitHub Check: build (ubuntu-20.04, 3.7)
- GitHub Check: build (ubuntu-20.04, 3.6)
🔇 Additional comments (3)
launchable/commands/subset.py (3)
135-141: LGTM: CLI option is well-defined.The
--non-blockingoption is properly implemented as a hidden option with clear help text.
650-651: LGTM: Helper function is well-defined.The
subset_requestfunction is properly implemented with correct type hints and encapsulates the request logic effectively.
533-540: 🛠️ Refactor suggestionAdd process error handling and cleanup.
The current implementation lacks proper process management:
- No error handling for process creation/start failures
- No cleanup of zombie processes
- No way to track the status of the subset request
if is_non_blocking: + try: # Create a new process for requesting a subset. - process = Process(target=subset_request, args=(client, timeout, payload)) - process.start() + process = Process(target=subset_request, args=(client, timeout, payload), daemon=True) + process.start() + # Store the process ID for potential future use + with open(os.path.join(app.get_metadata_dir(), f"subset_process_{session_id}.pid"), "w") as f: + f.write(str(process.pid)) + except Exception as e: + msg = f"Failed to start non-blocking subset request: {str(e)}" + click.echo(click.style(msg, fg="red"), err=True) + tracking_client.send_error_event( + event_name=Tracking.ErrorEvent.INTERNAL_CLI_ERROR, + stack_trace=str(e), + ) + sys.exit(1) click.echo("The subset was requested in non-blocking mode.", err=True) self.output_handler(self.test_paths, []) returnLikely invalid or redundant comment.
Summary
--non-blockingoption to thesubsetcommand@Konboi I've implemented this feature based on your suggestion. Can you review it? Thank you.
Summary by CodeRabbit