Modular wait for + Notebooks #5
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Summary of ChangesHello @LucaVor, 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 upgrades the Mantis SDK by introducing a more robust authentication mechanism that leverages cookies and space IDs to generate VSCode tokens. It also adds comprehensive capabilities for managing notebooks programmatically, allowing users to create, retrieve, and interact with notebook cells directly through the SDK. These changes are supported by updated documentation and examples, enhancing the SDK's usability and security for developers. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates to the Mantis SDK, focusing on notebook management and revised authentication. While the new features are a great addition, I've found a few critical issues in the client's networking logic that will likely cause all API requests to fail due to improper URL construction. There is also a high-severity issue in the new notebook functionality that could lead to an infinite loop. I've provided detailed comments and code suggestions to address these problems. Additionally, there are some medium-severity issues related to exception handling, a typo, and clarity in the documentation examples.
| def _authenticate(self): | ||
| """ | ||
| Authenticates by creating a VSCode token. | ||
| """ | ||
| try: | ||
| # We use the cookie to get the token | ||
| # The endpoint is /api/jupyter/vscode-auth-token/ | ||
| # It requires project_id in the body | ||
| url = f"{self.base_url.lstrip('/').rstrip('/')}/api/jupyter/vscode-auth-token/" | ||
| headers = {"cookie": self.cookie} | ||
| data = {"project_id": self.space_id} | ||
|
|
||
| response = requests.post(url, json=data, headers=headers) | ||
| response.raise_for_status() | ||
| result = response.json() | ||
|
|
||
| if result.get("success") and result.get("token"): | ||
| self.vscode_token = result.get("token") | ||
| logger.info("Successfully authenticated with VSCode token.") | ||
| else: | ||
| logger.warning(f"Failed to get VSCode token: {result}") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Authentication failed: {e}") |
There was a problem hiding this comment.
The _authenticate method has a couple of issues:
- It constructs a relative URL and uses
requests.postdirectly, which will fail. It should useself._requestto ensure the correct full URL is used and to centralize request logic. - It catches a broad
Exception, which can hide other bugs.
Here is a suggested refactoring that addresses both points by using the self._request method and catching a more specific exception.
| def _authenticate(self): | |
| """ | |
| Authenticates by creating a VSCode token. | |
| """ | |
| try: | |
| # We use the cookie to get the token | |
| # The endpoint is /api/jupyter/vscode-auth-token/ | |
| # It requires project_id in the body | |
| url = f"{self.base_url.lstrip('/').rstrip('/')}/api/jupyter/vscode-auth-token/" | |
| headers = {"cookie": self.cookie} | |
| data = {"project_id": self.space_id} | |
| response = requests.post(url, json=data, headers=headers) | |
| response.raise_for_status() | |
| result = response.json() | |
| if result.get("success") and result.get("token"): | |
| self.vscode_token = result.get("token") | |
| logger.info("Successfully authenticated with VSCode token.") | |
| else: | |
| logger.warning(f"Failed to get VSCode token: {result}") | |
| except Exception as e: | |
| logger.error(f"Authentication failed: {e}") | |
| def _authenticate(self): | |
| """ | |
| Authenticates by creating a VSCode token. | |
| """ | |
| try: | |
| data = {"project_id": self.space_id} | |
| result = self._request("POST", "/api/jupyter/vscode-auth-token/", json=data) | |
| if result.get("success") and result.get("token"): | |
| self.vscode_token = result.get("token") | |
| logger.info("Successfully authenticated with VSCode token.") | |
| else: | |
| logger.warning(f"Failed to get VSCode token: {result}") | |
| except RuntimeError as e: | |
| logger.error(f"Authentication failed: {e}") |
| return s.lstrip('/').rstrip('/') | ||
|
|
||
| url = f"{self.config.host}/{remove_slash(self.base_url)}/{remove_slash(endpoint)}/" | ||
| url = f"{remove_slash(self.base_url)}/{remove_slash(endpoint)}/" |
There was a problem hiding this comment.
The self.config.host component has been removed from the URL construction. This will result in relative URL paths being passed to requests, causing API calls to fail. This should be reverted to include the host, as in the previous version.
| url = f"{remove_slash(self.base_url)}/{remove_slash(endpoint)}/" | |
| url = f"{self.config.host}/{remove_slash(self.base_url)}/{remove_slash(endpoint)}/" |
| while True: | ||
| time.sleep(0.5) | ||
| self._refresh_content() | ||
|
|
||
| if index >= len(self.cells): | ||
| logger.warning(f"Cell index {index} out of range (cells count: {len(self.cells)}). Waiting for refresh...") | ||
| continue | ||
|
|
||
| cell = self.cells[index] | ||
| # check if executing | ||
| # note: metadata might be None | ||
| meta = cell.metadata or {} | ||
| if not meta.get("executing", False): | ||
| # execution finished | ||
| return cell.outputs |
There was a problem hiding this comment.
This while True loop polls for cell execution completion but lacks a timeout. If the backend process hangs or fails to update the 'executing' status, this will result in an infinite loop on the client side. You should add a timeout to prevent this, potentially using the timeout value from the client configuration.
| while True: | |
| time.sleep(0.5) | |
| self._refresh_content() | |
| if index >= len(self.cells): | |
| logger.warning(f"Cell index {index} out of range (cells count: {len(self.cells)}). Waiting for refresh...") | |
| continue | |
| cell = self.cells[index] | |
| # check if executing | |
| # note: metadata might be None | |
| meta = cell.metadata or {} | |
| if not meta.get("executing", False): | |
| # execution finished | |
| return cell.outputs | |
| start_time = time.time() | |
| timeout_seconds = self.client.config.timeout / 1000 # config.timeout is in ms | |
| while time.time() - start_time < timeout_seconds: | |
| time.sleep(0.5) | |
| self._refresh_content() | |
| if index >= len(self.cells): | |
| logger.warning(f"Cell index {index} out of range (cells count: {len(self.cells)}). Waiting for refresh...") | |
| continue | |
| cell = self.cells[index] | |
| # check if executing | |
| # note: metadata might be None | |
| meta = cell.metadata or {} | |
| if not meta.get("executing", False): | |
| # execution finished | |
| return cell.outputs | |
| raise RuntimeError(f"Cell execution timed out after {timeout_seconds} seconds.") |
|
|
||
| ```python | ||
| # Create a notebook | ||
| nb = mantis.create_notebook(space_id, "My Analysis", "user_id") |
There was a problem hiding this comment.
In this example snippet, the space_id variable is used but it has not been defined within this context. To make the example runnable and clearer for users, you should use new_space_id, which is created in the "Quick Start" example earlier in the document.
| nb = mantis.create_notebook(space_id, "My Analysis", "user_id") | |
| nb = mantis.create_notebook(new_space_id, "My Analysis", "user_id") |
| "render_args": RenderArgs(headless=True) | ||
| }) | ||
|
|
||
| mantis = MantisClient("/api/proxy/", cookie=cookie, space_id=space_id, config=config) |
There was a problem hiding this comment.
The space_id variable used here is not defined in the example. To improve clarity, you should either use new_space_id from the previous examples or a placeholder string like "YOUR_SPACE_ID_HERE".
| mantis = MantisClient("/api/proxy/", cookie=cookie, space_id=space_id, config=config) | |
| mantis = MantisClient("/api/proxy/", cookie=cookie, space_id=new_space_id, config=config) |
| reponse_content = None | ||
|
|
||
| try: | ||
| response = requests.request(method, url, headers=headers, **kwargs) | ||
| reponse_content = response.text | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.exceptions.RequestException as e: | ||
| raise RuntimeError(f"API request failed: {e}. Text: {response.text}") | ||
| # capture response text for better debugging | ||
| response_text = reponse_content if reponse_content else getattr(e.response, 'text', '') if e.response else str(e) | ||
| raise RuntimeError(f"API request failed: {e}. Text: {response_text}\nURL: {url}\nHeaders: {headers}") |
There was a problem hiding this comment.
There is a typo in the variable name reponse_content. It should be response_content for consistency and correctness. This typo appears on lines 133, 137, and 142.
| reponse_content = None | |
| try: | |
| response = requests.request(method, url, headers=headers, **kwargs) | |
| reponse_content = response.text | |
| response.raise_for_status() | |
| return response.json() | |
| except requests.exceptions.RequestException as e: | |
| raise RuntimeError(f"API request failed: {e}. Text: {response.text}") | |
| # capture response text for better debugging | |
| response_text = reponse_content if reponse_content else getattr(e.response, 'text', '') if e.response else str(e) | |
| raise RuntimeError(f"API request failed: {e}. Text: {response_text}\nURL: {url}\nHeaders: {headers}") | |
| response_content = None | |
| try: | |
| response = requests.request(method, url, headers=headers, **kwargs) | |
| response_content = response.text | |
| response.raise_for_status() | |
| return response.json() | |
| except requests.exceptions.RequestException as e: | |
| # capture response text for better debugging | |
| response_text = response_content if response_content else getattr(e.response, 'text', '') if e.response else str(e) | |
| raise RuntimeError(f"API request failed: {e}. Text: {response_text}\nURL: {url}\nHeaders: {headers}") |
| response = self.client._request("POST", "/api/sessions/check", json={"session_id": self.session_id, "project_id": self.space_id}) | ||
| if response.get("success"): | ||
| return | ||
| except Exception: |
There was a problem hiding this comment.
Catching a bare Exception is too broad and can hide unrelated bugs. It's better to catch a more specific exception that you expect here, for example RuntimeError which can be raised by self.client._request.
| except Exception: | |
| except RuntimeError: |
This pull request introduces significant updates to the Mantis SDK, focusing on improved authentication, notebook management, and enhanced documentation and examples. The most notable changes are the requirement of a
cookieandspace_idfor client authentication, the addition of notebook management methods, and corresponding updates to the documentation and example notebooks.Authentication and Client Initialization:
MantisClientnow requires both acookieand aspace_idupon initialization. These are used to authenticate the user and obtain a VSCode token for API requests. The authentication process is implemented within the client, and the VSCode token is included in subsequent requests for improved security and compatibility. [1] [2] [3] [4] [5]Notebook Management:
MantisClientfor creating and retrieving notebooks within a space:create_notebookandget_notebook. These facilitate programmatic notebook management and are documented in both the SDK and example notebooks. [1] [2] [3]Documentation and Example Updates:
README.mdandexamples/demo.ipynbhave been updated to reflect the new authentication requirements and demonstrate notebook creation and execution. These changes help users understand how to use the updated SDK features, including configuring the client and working with notebooks. [1] [2] [3] [4] [5] [6] [7] [8] [9]Additional API Utility:
resolve_map_to_projectmethod toMantisClientto resolve a map ID to a project ID, supporting more flexible project management workflows.These changes collectively enhance the SDK's usability, security, and documentation, making it easier for users to authenticate, configure, and manage their workspaces and notebooks programmatically.