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.
❌ Changes requested. Reviewed everything up to 2e46872 in 31 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted 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.
❌ Changes requested. Incremental review on e12b943 in 40 seconds
More details
- Looked at
127lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. tests/caching/test_result_store.py:196
- Draft comment:
Usingevalto load data is a security risk. Consider using a safer alternative likejson.loadsorast.literal_evalif the data format allows. - Reason this comment was not posted:
Marked as duplicate.
2. tests/caching/test_result_store.py:145
- Draft comment:
Avoid usingevalfor loading data as it can execute arbitrary code. Consider using a safer alternative likejson.loadsif 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.
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
pathargument. 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