-
Notifications
You must be signed in to change notification settings - Fork 14
Fix the condition for stats report to count the default load_module stats
#99
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
+ Coverage 83.16% 89.79% +6.63%
==========================================
Files 9 9
Lines 196 196
==========================================
+ Hits 163 176 +13
+ Misses 33 20 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR fixes a bug where default load_module statistics were not reported due to a misconfigured condition, and it enhances the unit tests to ensure this behavior works as expected.
- Updated the condition for reporting stats by removing the strict base_url check.
- Modified the parameter order for _download_via_git to align with its definition.
- Adjusted the return type and documentation for is_no_analytics in _conf.py.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_hub.py | Added a new unit test to ensure that _report_stats is invoked when the analytics are enabled. |
| optunahub/hub.py | Reordered parameters in the _download_via_git call and updated the is_official_registry condition. |
| optunahub/_conf.py | Updated is_no_analytics to always return a boolean and revised the corresponding docstring. |
Comments suppressed due to low confidence (3)
optunahub/hub.py:165
- The removal of the strict base_url comparison in the is_official_registry check alters the logic for reporting stats. Please confirm that this change is intended to support non-default base_url scenarios.
is_official_registry = repo_owner == "optuna" and repo_name == "optunahub-registry"
optunahub/hub.py:139
- The reordering of parameters in the _download_via_git call should be validated against the function definition to ensure consistency and clarity in API usage.
base_url=base_url or "https://github.com",
optunahub/_conf.py:34
- The function's return type and docstring have been updated to always return a boolean. Please verify that all callers of this function are compatible with this change.
def is_no_analytics() -> bool:
y0z
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.
LGTM
…patch Fix the condition for stats report to count the default `load_module` stats
Motivation
As the latest main branch does not count the stats for any default
load_modulecalls, we fix this bug.Description of the changes