-
Notifications
You must be signed in to change notification settings - Fork 755
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
fix: Path issues with pip modules #4256
base: main
Are you sure you want to change the base?
Conversation
…wanted behavior when other bentofiles are on the system PATH This is especially evident when trying to use bentoml cli on python folders that are also pip modules see bentoml#4217
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.
LGTM, thank you!
@@ -172,7 +169,7 @@ def recover_standalone_env_change(): | |||
object.__setattr__(instance, "_import_str", f"{module_name}:{attrs_str}") | |||
return instance | |||
except ImportServiceError: | |||
if sys_path_modified and working_dir: | |||
if working_dir: |
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.
we probably don't need this check neither now?
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.
I think this check is ensuring that if the assignment fails it doesn't try to remove None
.
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.
Make sense, I'd suggest we move the working_dir assignment L75-81 out of the try
block and remove this if working_dir
check as well
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.
@sauyon made a quick patch, could you help take a look?
we probably should note the side effect of doing this here
|
Always prepend current bentofile directory to system path to avoid unwanted behavior when other bentofiles are on the system PATH
This is especially evident when trying to use bentoml cli on python folders that are also pip modules see #4217