add info if package is missing#741
Conversation
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds explicit ConnectionError propagation in lease connection wait and introduces ModuleNotFoundError handling in dynamic class import to surface descriptive ConnectionError messages for missing Jumpstarter drivers or Python modules; plus minor formatting and logging cleanups. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Lease as Lease._wait_for_ready_connection
participant Remote as RemoteService
Client->>Lease: call _wait_for_ready_connection()
Lease->>Remote: attempt connect
alt ConnectionError from Remote
Remote-->>Lease: ConnectionError
Lease-->>Client: re-raise ConnectionError (propagate)
else Other Exception
Remote-->>Lease: Error
Lease-->>Client: wrap/handle generic Exception
end
sequenceDiagram
autonumber
participant Importer as import_class
participant Cache as cached_import
Importer->>Cache: request module/class
Cache-->>Importer: throws ModuleNotFoundError
alt module name starts with "jumpstarter_"
Importer->>Importer: log exporter-version mismatch
Importer-->>Caller: raise ConnectionError (exporter mismatch)
else non-driver module
Importer->>Importer: log missing python dependency
Importer-->>Caller: raise ConnectionError (install dependency)
end
Cache-->>Importer: returns module/class (normal path)
Importer-->>Caller: return imported class
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/client/lease.py (1)
244-270: Prefer usingModuleNotFoundError.name.Parsing the human-readable message is fragile; the exception already exposes the missing module via
e.name, which handles alias cases and avoids splitting assumptions.Apply this diff to rely on the structured attribute with a safe fallback:
- except ModuleNotFoundError as e: - module_name = str(e).split("'")[1] if "'" in str(e) else str(e).split()[-1] + except ModuleNotFoundError as e: + message = str(e) + module_name = getattr(e, "name", None) + if not module_name: + module_name = message.split("'")[1] if "'" in message else message.split()[-1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 589
File: packages/jumpstarter/jumpstarter/exporter/exporter.py:147-155
Timestamp: 2025-08-14T13:11:35.034Z
Learning: In the jumpstarter fork-based architecture, when the exporter's serve() method exits (e.g., due to lease changes), the child process terminates and the parent process automatically restarts it, eliminating concerns about orphaned tasks since the entire process is restarted.
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/client/lease.py
🧬 Code graph analysis (1)
packages/jumpstarter/jumpstarter/client/lease.py (2)
packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ConnectionError(42-45)packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
| else: | ||
| logger.warning("Waiting for ready connection to %s: %s", path, e) | ||
| await sleep(5) | ||
| except ModuleNotFoundError as e: |
There was a problem hiding this comment.
I would do this on the import place in the code, because it could be that the driver is not from out jumpstarter_xxx namespace
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/common/importlib.py (1)
67-73: Consider clarifying that module and package names may differ.The error message assumes the module name matches the pip package name, which isn't always true (e.g.,
cv2requiresopencv-python,PILrequiresPillow). While this guidance is helpful in most cases, consider adding a note about potential name differences.Apply this diff to clarify:
raise ConnectionError( f"Missing Python module '{module_name}'.\n\n" - "Please install the missing module:\n" + "Please install the missing module (note: package name may differ from module name):\n" f" pip install {module_name}\n\n" "or if using uv:\n" f" uv pip install {module_name}" ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/common/importlib.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/client/lease.py
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ConnectionError(42-45)
packages/jumpstarter/jumpstarter/common/importlib.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ConnectionError(42-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter/jumpstarter/common/importlib.py (1)
3-8: LGTM!Adding logging support enables structured error reporting for dynamic imports.
packages/jumpstarter/jumpstarter/client/lease.py (5)
244-245: LGTM! Proper ConnectionError propagation.Explicitly re-raising
ConnectionErrorensures that the descriptive error messages from the import layer (e.g., missing jumpstarter drivers or Python modules) are preserved and not wrapped in a generic connection error.
257-272: LGTM! Formatting improvements.The reformatting improves readability without changing functionality.
325-329: LGTM! Readability improvement.Multi-line formatting makes the compound boolean condition easier to read.
335-335: LGTM! Consolidated formatting.Single-line formatting for the short argument list is appropriate.
349-361: LGTM! Consistent elapsed time formatting.Using
split(".")[0]to remove microseconds is consistently applied across all locations (lines 349, 354, 361), improving code uniformity.
| except ModuleNotFoundError as e: | ||
| module_name = str(e).split("'")[1] if "'" in str(e) else str(e).split()[-1] |
There was a problem hiding this comment.
Use ModuleNotFoundError's .name attribute instead of string parsing.
The current string parsing approach is fragile and can fail if the error message format changes. ModuleNotFoundError provides a .name attribute that directly contains the module name.
Apply this diff to use the more reliable approach:
except ModuleNotFoundError as e:
- module_name = str(e).split("'")[1] if "'" in str(e) else str(e).split()[-1]
+ module_name = e.name if e.name else "unknown"📝 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.
| except ModuleNotFoundError as e: | |
| module_name = str(e).split("'")[1] if "'" in str(e) else str(e).split()[-1] | |
| except ModuleNotFoundError as e: | |
| module_name = e.name if e.name else "unknown" |
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/common/importlib.py around lines 42-43,
replace the fragile string-parsing that extracts the missing module name from
ModuleNotFoundError with the built-in attribute: use the exception's .name
property (e.name) to obtain the module name and assign that to module_name; keep
behavior the same if .name is None by falling back to str(e) only if necessary.
2 participants
address #733
Summary by CodeRabbit
Bug Fixes
Style / Minor UX