Mask input dimensionality #26
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @safaricd, 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 introduces a mechanism to anonymize dataset dimensionality in telemetry data, ensuring user privacy and GDPR compliance while still allowing for valuable insights into general dataset characteristics.
Highlights
- New Function
_round_dims: A new private function_round_dimshas been added todecorators.pyto round dataset dimensions for telemetry purposes. - Telemetry Anonymization: This function anonymizes dataset shapes (e.g., (953, 17) becomes (1000, 20)) by rounding them to predefined ranges, preventing leakage of sensitive information while preserving useful insights.
- Integration into Telemetry Pipeline: The
_make_callinfofunction now utilizes_round_dimsto process dataset shapes before they are included in telemetry data. - Comprehensive Testing: A new test file
test_decorators.pyhas been added with extensive unit tests for the_round_dimsfunction, covering various rounding scenarios, edge cases, and ensuring its anonymization purpose. - Telemetry Event Property Fix: The
propertiesmethod inevents.pyhas been updated to correctly include thesourceproperty in telemetry event dictionaries.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for anonymizing dataset dimensionality for telemetry, which is great for privacy. The implementation is mostly solid, and the addition of comprehensive tests is commendable. However, I've identified a critical issue where the new rounding function is called without proper handling of potential None values, which could lead to runtime exceptions. I've also pointed out some confusing logic in the rounding function and a misleading test name that should be addressed to improve code clarity and robustness. My suggestions aim to fix the critical bug and enhance the maintainability of the new code.
| # Round the dimensionality of the dataset | ||
| shape = _round_dims(shape_of(bound.arguments[param_name])) | ||
| if shape is not None: | ||
| shapes[param_name] = shape |
There was a problem hiding this comment.
There's a potential TypeError here. The previous implementation correctly checked if the result from shape_of() was None before using it. The new code passes the result directly to _round_dims(), which does not handle None as input. If shape_of() can return None (as the original check implies), this will raise an exception at runtime. The check for None should be performed before calling _round_dims() to prevent this.
| # Round the dimensionality of the dataset | |
| shape = _round_dims(shape_of(bound.arguments[param_name])) | |
| if shape is not None: | |
| shapes[param_name] = shape | |
| raw_shape = shape_of(bound.arguments[param_name]) | |
| if raw_shape is not None: | |
| # Round the dimensionality of the dataset | |
| shapes[param_name] = _round_dims(raw_shape) |
| if not tuple(shape): | ||
| return 0, 0 |
There was a problem hiding this comment.
This check for an empty shape is confusing and should be removed for the following reasons:
- The
tuple(shape)conversion is redundant sinceshapeis already a tuple.if not shape:would be more idiomatic. - The function's type hint is
tuple[int, int], which means an empty tuple()should not be a valid input. If it were possible, it would be a type violation. - This logic is not covered by tests, suggesting it might be dead code.
- If
shape_ofis the only source for theshapeargument, it seems to always return a 2-element tuple, so this condition will never be met.
Removing these lines will make the code clearer and align it better with its type hints and intended usage.
| assert _round_dims((156, 60)) == (200, 75) # 156 -> 200, 60 -> 75 | ||
| assert _round_dims((953, 17)) == (1000, 20) # 953 -> 1000, 17 -> 20 | ||
|
|
||
| def test_round_dims_empty_shape(self) -> None: |
There was a problem hiding this comment.
The test name test_round_dims_empty_shape is misleading as it tests a zero-sized shape (0, 0) rather than an empty shape (). This can create confusion about the function's behavior, especially given the related ambiguity in the _round_dims implementation itself. Renaming the test to more accurately reflect what it's testing would improve clarity for future maintenance.
| def test_round_dims_empty_shape(self) -> None: | |
| def test_round_dims_zero_shape(self) -> None: |
Change Description
This change introduces the
_round_dimsfunction to anonymize dataset shapes for telemetry purposes, ensuring GDPR compliance by masking dimensionality details. Instead of logging exact values like(953, 17), the function rounds dimensions to predefined ranges (e.g., (1000, 20)), effectively obfuscating sensitive information while preserving useful insights about dataset characteristics.This approach allows us to gather meaningful telemetry data about the types of datasets users are working with - including size ranges and complexity patterns - without violating privacy or exposing potentially sensitive information about specific datasets.