Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello, 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 enhances the telemetry system by introducing a new event, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new extension_entry telemetry event, which is designed to fire only once per extension instantiation. The implementation looks solid, with a new event class, modifications to the @set_extension decorator to fire the event, and comprehensive new tests for both the event and the decorator logic. However, I've noticed some potentially unintended changes in the uv.lock file. The version constraints for pandas, posthog, and typing-extensions have been relaxed, removing upper bounds. This could lead to future breakages if new major versions of these libraries with incompatible changes are released. I've added specific comments on these lines. Additionally, I've suggested a small improvement to a test name for clarity.
| { name = "numpy", specifier = ">=1.21.6" }, | ||
| { name = "nvidia-ml-py", specifier = ">=13.590.48" }, | ||
| { name = "pandas", specifier = ">=1.4.0,<3" }, | ||
| { name = "pandas", specifier = ">=1.4.0" }, |
There was a problem hiding this comment.
The upper bound for the pandas dependency has been removed, changing from >=1.4.0,<3 to >=1.4.0. This allows pandas version 3.0 and higher, which may introduce breaking changes. To prevent potential compatibility issues, it is recommended to restore the upper bound unless compatibility with pandas v3 has been verified and is intended.
| { name = "pandas", specifier = ">=1.4.0" }, | ||
| { name = "platformdirs", specifier = ">=4" }, | ||
| { name = "posthog", specifier = "~=6.7" }, | ||
| { name = "posthog", specifier = ">=6.7" }, |
There was a problem hiding this comment.
The version constraint for posthog has been relaxed from ~=6.7 to >=6.7. The ~= operator (equivalent to >=6.7, <7.0) protects against breaking changes in new major versions. The new constraint >=6.7 allows any future version, including major versions that may not be compatible. It is recommended to use a more restrictive constraint like ~=6.7 to ensure stability.
| { name = "scikit-learn", specifier = ">=1.2.0" }, | ||
| { name = "twine", marker = "extra == 'build'", specifier = ">=5.0.0" }, | ||
| { name = "typing-extensions", specifier = ">=4.12,<5" }, | ||
| { name = "typing-extensions", specifier = ">=4.12" }, |
There was a problem hiding this comment.
The upper bound for typing-extensions has been removed, changing from >=4.12,<5 to >=4.12. This could allow version 5.x, which might introduce breaking API changes. To prevent potential issues, it's advisable to restore the upper bound unless compatibility with the new major version has been verified and is intended.
| assert mock_capture.call_args[0][0].extension_name == "cls_ext" | ||
|
|
||
| @patch("tabpfn_common_utils.telemetry.core.decorators.capture_event") | ||
| def test_event_not_emitted_on_exception(self, mock_capture): |
There was a problem hiding this comment.
The name of this test, test_event_not_emitted_on_exception, is a bit misleading. It suggests that an exception within the decorated function would prevent event emission. However, the test actually verifies that an exception during the capture_event call does not crash the application and allows the decorated function to run. A more descriptive name like test_function_executes_when_capture_event_fails would better reflect the test's purpose.
b4fc9f7 to
bf3db80
Compare
bf3db80 to
fb600a6
Compare
Adding a new
extension_entryevent that only fires once per instantiation, to get a feel for extension usage regardless of number offit/predictcalls.Will bump version in extensions and create posthog insight afterwards.