-
Notifications
You must be signed in to change notification settings - Fork 133
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
Fixes #1240 #1241
Fixes #1240 #1241
Conversation
The cache store assumed that every persister took a `path` argument. That is not the case because the savers / loaders wrap external APIs and we decided to not try to create our own abstraction layer around them, and instead mirror them. E.g. polars takes `file`, but pandas takes `path`.
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.
❌ Changes requested. Reviewed everything up to 2e46872 in 31 seconds
More details
- Looked at
36
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. hamilton/caching/stores/file.py:80
- Draft comment:
Ensure thatsaver_cls.name()
andloader_cls.name()
are valid method calls. If these classes do not have aname()
method, consider usingsaver_cls.__name__
andloader_cls.__name__
to get the class name. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_9zJcc6qIk9G7I0QK
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.
To catch case with `file` and without `path` or `file`.
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.
❌ Changes requested. Incremental review on e12b943 in 40 seconds
More details
- Looked at
127
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. tests/caching/test_result_store.py:196
- Draft comment:
Usingeval
to load data is a security risk. Consider using a safer alternative likejson.loads
orast.literal_eval
if the data format allows. - Reason this comment was not posted:
Marked as duplicate.
2. tests/caching/test_result_store.py:145
- Draft comment:
Avoid usingeval
for loading data as it can execute arbitrary code. Consider using a safer alternative likejson.loads
if the data format allows. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ZHnp8LhCA8wQPFRN
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.
|
||
def load_data(self, type_: Type[Type]) -> Tuple[Type, Dict[str, Any]]: | ||
with open(self.file, "r") as f: | ||
data = eval(f.read()) |
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.
Using eval
to load data is a security risk. Consider using a safer alternative like json.loads
or ast.literal_eval
if the data format allows.
data = eval(f.read()) | |
data = ast.literal_eval(f.read()) |
The cache store assumed that every persister took a
path
argument. That is not the case because the savers / loaders wrap external APIs and we decided to not try to create our own abstraction layer around them, and instead mirror them.E.g. polars takes
file
, but pandas takespath
.Changes
How I tested this
Notes
Checklist