-
Notifications
You must be signed in to change notification settings - Fork 3
Fix input validation and clean up minor code issues #681
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors input validation to use explicit exceptions instead of assertions, removes a redundant assignment, centralizes the default cache size in a constant, and makes the rerun viewer URL dynamically use the installed rerun package version with a safe fallback. Sequence diagram for dynamic rerun viewer version resolutionsequenceDiagram
participant Caller
participant UtilsRerun
participant ImportlibMetadata as ImportlibMetadata
participant Panel
Caller->>UtilsRerun: rrd_to_pane(url, width, height, version=None)
UtilsRerun->>UtilsRerun: _get_rerun_version()
UtilsRerun->>ImportlibMetadata: get_package_version(rerun-sdk)
alt version lookup succeeds
ImportlibMetadata-->>UtilsRerun: rerun_version
else version lookup fails
ImportlibMetadata-->>UtilsRerun: raises Exception
UtilsRerun->>UtilsRerun: use fallback 0.20.1
end
UtilsRerun->>Panel: pn.pane.HTML(iframe_html)
Panel-->>Caller: HTMLPane
Class diagram for updated Bencher initialization and rerun utilitiesclassDiagram
class BencherModule {
+ DEFAULT_CACHE_SIZE_BYTES int
}
class Bencher {
- bench_name str
- worker Any
- worker_class_instance Any
- worker_input_cfg Any
- cache_size int
- sample_cache Any
- ds_dynamic dict
+ __init__(bench_name, worker, worker_input_cfg, run_cfg, report)
+ set_worker(worker, worker_input_cfg)
+ plot_sweep(input_vars_in, run_cfg)
}
class UtilsRerun {
+ _get_rerun_version() str
+ rrd_to_pane(url str, width int, height int, version str) HTMLPane
}
BencherModule --> Bencher : uses DEFAULT_CACHE_SIZE_BYTES
UtilsRerun --> Bencher : used in plotting workflows
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- In
_get_rerun_version, catching a broadExceptionwill hide unexpected issues; consider catchingimportlib.metadata.PackageNotFoundError(and possibly logging a warning) before falling back to the hardcoded version. - Since
DEFAULT_CACHE_SIZE_BYTESencodes the 100 GB default, you might want to express it as100 * 1024**3(and/or update the nearby comment to derive from the constant) to avoid ambiguity if the value changes later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_get_rerun_version`, catching a broad `Exception` will hide unexpected issues; consider catching `importlib.metadata.PackageNotFoundError` (and possibly logging a warning) before falling back to the hardcoded version.
- Since `DEFAULT_CACHE_SIZE_BYTES` encodes the 100 GB default, you might want to express it as `100 * 1024**3` (and/or update the nearby comment to derive from the constant) to avoid ambiguity if the value changes later.
## Individual Comments
### Comment 1
<location> `bencher/utils_rerun.py:11-13` </location>
<code_context>
+def _get_rerun_version() -> str:
+ """Get the installed rerun package version."""
+ try:
+ return get_package_version("rerun-sdk")
+ except Exception:
+ return "0.20.1" # Fallback version
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Narrow the exception type when resolving the rerun package version to avoid masking unrelated errors.
Catching `Exception` here can mask real problems (e.g., import errors or environment misconfiguration). Please catch `importlib.metadata.PackageNotFoundError` for the missing-package case instead, and let other exceptions propagate (optionally with a log warning before falling back to the hard-coded version).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
blooop
left a comment
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.
Thanks!
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Fixes a few small code issues:
Input validation: Changed
assertstatements to raiseTypeError/ValueErrorwith descriptive messages. Assertions can be disabled with-O, so they shouldn't be used for input validation.Dead code: Removed duplicate
self.worker_class_instance = Noneassignment.Magic number: Extracted the 100GB cache size to
DEFAULT_CACHE_SIZE_BYTESconstant.Hardcoded version: The rerun viewer URL now uses the installed package version via
importlib.metadatainstead of a hardcoded string (resolves the TODO).Summary by Sourcery
Improve input validation and configuration around benchmarking cache and rerun viewer integration.
Bug Fixes:
Enhancements: