Add SDK config #1232
Add SDK config #1232
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to fc6093a in 44 seconds
More details
- Looked at
255lines of code in6files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. ui/sdk/src/hamilton_sdk/tracking/constants.py:28
- Draft comment:
The_load_configfunction is repeated in multiple files. Consider refactoring to maintain a single implementation to adhere to the DRY principle. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a potential DRY violation, which is a valid concern. However, without evidence from the diff that the function is repeated, the comment is speculative. The comment should be removed unless there is strong evidence within the diff itself.
I might be missing the context from other files where the function is repeated. However, the rules state to focus only on the file in the diff.
The rules are clear about focusing on the file in the diff, so without evidence in this file, the comment should be removed.
Remove the comment as it is speculative and not supported by evidence in the diff.
Workflow ID: wflow_9ePQ1BbWNVunb0N6
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 76606ee in 3 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. tests/plugins/test_plotly_extensions.py:19
- Draft comment:
Consider adding a comment explaining why settingBROWSER_PATHto an empty string is necessary forPlotlyStaticWriterto work. This will help future developers understand the context. - Reason this comment was not posted:
Confidence changes required:50%
The environment variable 'BROWSER_PATH' is set to an empty string to make PlotlyStaticWriter work. This should be documented or explained in the code comments for clarity.
2. tests/plugins/test_plotly_extensions.py:19
- Draft comment:
Avoid setting environment variables likeBROWSER_PATHdirectly in the code without proper documentation or explanation, as it might be considered sensitive or unnecessary. Consider adding a comment explaining why this is safe or necessary. - Reason this comment was not posted:
Confidence changes required:50%
The code sets an environment variableBROWSER_PATHto an empty string, which might be considered sensitive or unnecessary if not properly documented. However, since this is a test file, it might be acceptable, but a comment explaining why this is safe or necessary would be helpful.
Workflow ID: wflow_HyUqsLgX9Vo6blrB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 09491de in 16 seconds
More details
- Looked at
74lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. ui/sdk/tests/tracking/test_constants.py:5
- Draft comment:
Consider adding a test case for an empty string to ensure_convert_to_typehandles it correctly. - Reason this comment was not posted:
Confidence changes required:50%
The test function is well-structured and covers various cases for the_convert_to_typefunction. However, it would be beneficial to add a test case for an empty string to ensure the function handles it correctly.
2. ui/sdk/src/hamilton_sdk/tracking/constants.py:74
- Draft comment:
Consider using_convert_to_typeforCAPTURE_DATA_STATISTICSto ensure consistent type conversion logic.
CAPTURE_DATA_STATISTICS = _convert_to_type(os.getenv("HAMILTON_CAPTURE_DATA_STATISTICS", CAPTURE_DATA_STATISTICS))
- Reason this comment was not posted:
Confidence changes required:50%
The current implementation of environment variable handling forCAPTURE_DATA_STATISTICSis not consistent with the_convert_to_typefunction. It would be better to use_convert_to_typefor consistency and to handle potential future changes in type conversion logic.
3. ui/sdk/tests/tracking/test_constants.py:5
- Draft comment:
Function nametest__convert_to_typeshould follow a consistent naming pattern. Consider renaming it to something more descriptive and consistent with other test function names. - Reason this comment was not posted:
Confidence changes required:50%
The function_convert_to_typeis used to convert string values to their appropriate types. This function is tested intest__convert_to_type, which is good. However, the function nametest__convert_to_typedoes not follow a consistent naming pattern with the rest of the codebase. It should be more descriptive and consistent with other test function names.
Workflow ID: wflow_PvxHwUKjsFDIIWzr
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 1decd00 in 12 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/plugins/test_plotly_extensions.py:19
- Draft comment:
Avoid hardcoding sensitive paths like BROWSER_PATH. Consider using a configuration file or environment variable. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_PplrWqit4WSxXY3S
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f890278 in 15 seconds
More details
- Looked at
35lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. ui/sdk/src/hamilton_sdk/tracking/constants.py:45
- Draft comment:
Consider logging the exception in the_load_configfunction to help with debugging if the config file cannot be read. - Reason this comment was not posted:
Confidence changes required:50%
The code is mostly well-structured, but there are some areas for improvement.
2. ui/sdk/src/hamilton_sdk/tracking/constants.py:48
- Draft comment:
The function_convert_to_typeis well-implemented, but consider adding type hints for better clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:30%
The code is mostly well-structured, but there are some areas for improvement.
Workflow ID: wflow_w0enUsxTnfM6Eiju
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Looking reasonable, let's:
- SImplify the way we use globals
- Add docs
Will take another quick look after docs then let's merge.
Also, some features we'll want down the road:
- Turn on/off per-node/type/attribute name
- Add a max-payload size, where it gets dropped afterwards
Although (2) is a bit more complicated.
These should be in the docs IMO as next steps.
| return config | ||
|
|
||
|
|
||
| _constant_values = globals() |
There was a problem hiding this comment.
I feel like you should just hardcode this to a dict then we don't have to deal with globals. E.G.
constants_values={"CAPTURE_DATA_STATISTICS" : True, ...}
Then you just load up the file and modify that. Should simplify the code.
There was a problem hiding this comment.
No I don't like that because that would then encumber directly accessing the module values; or I could do something like this at the end to push them all into the module space... either way playing with globals.
There was a problem hiding this comment.
This has a single global versus multiple? Don't see why we want to directly access module values.
This is for flexibility in determining where to look for the config file.
skrawcz
force-pushed
the
add_sdk_config
branch
from
f890278
to
80c24fa
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ff484f5 in 3 seconds
More details
- Looked at
100lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docs/hamilton-ui/ui.rst:308
- Draft comment:
Typo: 'prescedence' should be 'precedence'.
precedence
- Reason this comment was not posted:
Confidence changes required:10%
The documentation is clear and provides detailed instructions on how to configure the SDK. However, there is a minor typo in the word 'precedence'.
2. docs/hamilton-ui/ui.rst:239
- Draft comment:
This section on SDK Configuration is a valuable addition to the documentation. It provides clear instructions on how to configure the HamiltonTracker, which is beneficial for users. Ensure this is included in the Sphinx documentation. - Reason this comment was not posted:
Confidence changes required:0%
The documentation added in this PR is comprehensive and provides clear instructions on SDK configuration. It is appropriate to include this in the Sphinx documentation as it enhances user understanding of configuration options.
Workflow ID: wflow_C3u8h0DLm7G50ZOO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 25ace32 in 5 seconds
More details
- Looked at
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docs/hamilton-ui/ui.rst:278
- Draft comment:
Add a newline before the code block for better readability.
.. code-block:: bash
- Reason this comment was not posted:
Confidence changes required:10%
The code block formatting is inconsistent in the added sections. Adding a newline before the code block can improve readability.
2. docs/hamilton-ui/ui.rst:306
- Draft comment:
Consider adding a section in the Sphinx documentation underdocs/to explain the new SDK configuration options and how to use them. This will help users understand the new features and how to configure them effectively. - Reason this comment was not posted:
Confidence changes required:50%
The documentation update is appropriate for the changes made in the PR. It explains how to configure the SDK using different methods, which aligns with the changes made in the code.
Workflow ID: wflow_iEZJjJDA7dtHg5xt
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
skrawcz
force-pushed
the
add_sdk_config
branch
from
25ace32
to
c9a855b
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c9a855b in 3 seconds
More details
- Looked at
64lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docs/hamilton-ui/ui.rst:308
- Draft comment:
Add a blank line before the code block for better readability. - Reason this comment was not posted:
Confidence changes required:10%
The documentation is clear and provides detailed instructions on how to change the configuration settings. However, there is a minor formatting issue in the code block for changing defaults via environment variables and directly. Adding a blank line before the code block would improve readability.
2. docs/hamilton-ui/ui.rst:241
- Draft comment:
Consider adding a section here to document the new configuration options forCAPTURE_DATA_STATISTICS,MAX_LIST_LENGTH_CAPTURE, andMAX_DICT_LENGTH_CAPTUREas they are part of the new SDK configuration options. - Reason this comment was not posted:
Confidence changes required:50%
The documentation update is appropriate for the changes made in the PR. It explains the new configuration options and how to use them.
Workflow ID: wflow_Qu8TXAszeDsvqkEo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
skrawcz
force-pushed
the
add_sdk_config
branch
from
c9a855b
to
498068e
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 498068e in 2 seconds
More details
- Looked at
93lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docs/hamilton-ui/ui.rst:280
- Draft comment:
The table alignment directive:align: leftis not necessary and can be removed for cleaner formatting. - Reason this comment was not posted:
Confidence changes required:10%
The documentation is clear and well-structured, but there is a minor formatting issue with the table alignment.
2. docs/hamilton-ui/ui.rst:241
- Draft comment:
Consider adding a reference to this section in the main documentation index to improve discoverability. - Reason this comment was not posted:
Confidence changes required:20%
The documentation update is appropriate and provides clear instructions on SDK configuration. It aligns with the changes made in the code.
Workflow ID: wflow_sZbYU0azgyFHoNxt
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3a6a5c1 in 34 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. ui/sdk/src/hamilton_sdk/tracking/utils.py:50
- Draft comment:
Consider adding a comment here to explain that the.head()method is used to truncate dataframes to their head rows, as indicated in the test file. - Reason this comment was not posted:
Confidence changes required:50%
The comment in the test file indicates that the.head()method is being used to truncate data, but this is not documented in the main code. A comment should be added to the main code to explain this behavior.
Workflow ID: wflow_BZhQXixtVXdUlOXL
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
skrawcz
force-pushed
the
add_sdk_config
branch
from
3a6a5c1
to
c7fe7c0
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on c7fe7c0 in 26 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. ui/sdk/tests/tracking/test_utils.py:139
- Draft comment:
The test does not account for the.head()call inmake_json_safe. Update the expected result to match the truncated series output. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The test case already has a comment explaining the skipped element due to.head(), and the expected result matches this explanation. The automated comment seems redundant as the change is already reflected in the test case.
I might be missing some context about themake_json_safefunction's behavior, but based on the test case, it seems the expected result is already correct.
The test case's comment and expected result align, indicating the change is already accounted for. The automated comment does not provide new information.
The automated comment is unnecessary as the test case already accounts for the.head()call, and the expected result is correct.
Workflow ID: wflow_9QpLRVxNl8Dwtkft
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
This adds a few things to enable one to more easily customize what is captured via the SDK, and adds docs. 1. Should you capture any data statistics at all? 2. Max lengths of dicts and lists -> so they don't get too big. You can configure this via three means: 1. python constants 2. config file variables 3. environment variables There is a precedence order. So there are default values in the module. You can then override them via config file variables. Which then can in turn be overridden by environment variables. Lastly the user can always modify the constants by directly changing the module variables. Note: we also skip logging metadata from datasavers and loaders if the CAPTURE_DATA_STATISTICS = False. We can fix this by special casing it, but for now I don't think people would complain with the current functionality. We use globals and modify state of the constants to help ensure a great developer experience / static analysis is able to find where people are using the constants easily. Squashed commits: Tweaks docs on sdk constants (+4 squashed commits) Squashed commits: [ff484f5] Adds some docs on the SDK configurability [80c24fa] minor refactor of constants function [69d12f3] Adds function to convert config types Config parser types are by default strings. They aren't converted. This adds a function to do that for ints, floats, and booleans. [0a4949e] Adds configurable capture for SDK This adds a few things to enable one to more easily customize what is captured via the SDK. 1. Should you capture any data statistics at all? 2. Max lengths of dicts and lists -> so they don't get too big. You can configure this via three means: 1. python constants 2. config file variables 3. environment variables There is a prescedence order. So there are default values in the module. You can then override them via config file variables. Which then can in turn be overriden by environment variables. Lastly the user can always modify the constants by directly changing the module variables. Note: we also skip logging metadata from datasavers and loaders if the CAPTURE_DATA_STATISTICS = False. We can fix this by special casing it, but for now I don't think people would complain with the current functionality.
This should ideally use the same data observability introspection code path. It doesn't. So this is a stop gap measure to log the head rows of a dataframe if you use one as input.
skrawcz
force-pushed
the
add_sdk_config
branch
from
c7fe7c0
to
4088955
Compare
This adds a few things to enable one to more
easily customize what is captured via the SDK.
You can configure this via three means:
There is a prescedence order. So there are default values in the module.
You can then override them via config file variables.
Which then can in turn be overriden by environment variables.
Lastly the user can always modify the constants by directly
changing the module variables.
Note: we also skip logging metadata from datasavers and loaders if
the CAPTURE_DATA_STATISTICS = False. We can fix this by special
casing it, but for now I don't think people would complain with
the current functionality.
We should still enable #921 but I think this is a simpler route for now.
Changes
How I tested this
Notes
This is related to:
Checklist