Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Path issues with pip modules #4256

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: Path issues with pip modules #4256

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
544d47e
Always prepend current bentofile directory to system path to avoid un…
tokotchd Nov 2, 2023
b0ba2ab
Merge branch 'main' into fix/pip_modules
aarnphm Nov 3, 2023
21ab30e
Update loader.py
parano Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/bentoml/_internal/service/loader.py
View file
Open in desktop
Expand Up @@ -63,7 +63,6 @@ def import_service(
from bentoml import Service

prev_cwd = None
sys_path_modified = False
prev_cwd = os.getcwd()
global_model_store = BentoMLContainer.model_store.get()

Expand All @@ -81,9 +80,7 @@ def recover_standalone_env_change():
else:
working_dir = os.getcwd()

if working_dir not in sys.path:
sys.path.insert(0, working_dir)
sys_path_modified = True
sys.path.insert(0, working_dir)

if model_store is not global_model_store:
BentoMLContainer.model_store.set(model_store)
Expand Down Expand Up @@ -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:
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Member

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?

# Undo changes to sys.path
sys.path.remove(working_dir)

Expand Down