Fix a bug where we couldn't execute imported functions #259
Conversation
|
|
||
| # work_dir should be `<storage>/operators/<id>/op` | ||
| work_dir = os.path.join(fn_path, OP_DIR) | ||
| print("listdir workdir") |
There was a problem hiding this comment.
Let's be a bit more descriptive here: "Listing files under the current working directory." The reason being I feel others may have a hard time understanding what listdir workdir means.
There was a problem hiding this comment.
I agree. I think it is kind of confusing what listdir and workdir represent, might need some documentation or clarification there.
| print("listdir fn path") | ||
| print(os.listdir(fn_path)) |
There was a problem hiding this comment.
It seems that the purpose of fn_path is just to construct the work_dir which is what we need, so I don't see a strong motivation for us to print out fn_path anymore.
| import_path = _get_py_import_path(spec) | ||
| print(f"import_path: {import_path}") |
There was a problem hiding this comment.
I'd suggest using a full sentence for clarity: f"The model import path is: {import_path}."
|
|
||
|
|
||
| def _import_invoke_method(spec: FunctionSpec) -> Callable[..., DataFrame]: | ||
| # `_import_invoke_method` imports the model object. | ||
| # it assumes the operator has been extracted to `<storage>/operators/<id>/op` | ||
| # and imports the route from the above path. |
There was a problem hiding this comment.
what does "route" mean here?
nit: can we use the """ format for docstrings? I looks like its the standard and also looks way better on my IDE :D https://peps.python.org/pep-0257/
| print("listdir fn path") | ||
| print(os.listdir(fn_path)) | ||
| os.chdir(work_dir) | ||
| sys.path.append(work_dir) |
There was a problem hiding this comment.
Can we comment this line? I think its important that a reader knows what the purpose of this specific line is, and what failure case can occur when its not included.
| print("listdir workdir") | ||
| print(os.listdir(work_dir)) | ||
| print("listdir fn path") | ||
| print(os.listdir(fn_path)) |
There was a problem hiding this comment.
I have a tiny tiny readability preference to merge the prefix and the list into one line ala "listdir(fn_path): %s" % os.listdir(fn_path)
|
|
||
| # work_dir should be `<storage>/operators/<id>/op` | ||
| work_dir = os.path.join(fn_path, OP_DIR) | ||
| print("listdir workdir") |
There was a problem hiding this comment.
I agree. I think it is kind of confusing what listdir and workdir represent, might need some documentation or clarification there.
Describe your changes and why you are making these changes
This PR fixes the bug that we couldn't execute imported functions like the following:
Specifically, we fixed the
sys.pathwhich we used to import user functions to make sure the dependencies also properly appears insys.path. We also added a few debugging logs for the extracted operator files.Related issue number (if any)
ENG-1451
Checklist before requesting a review
Verified the code snippet above works after fix
python3 scripts/run_linters.py -hfor usage).run_integration_test: Runs integration testsskip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)