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.

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

Disable alter_sys_path on background threads. #3489

Closed
wants to merge 1 commit into from

Conversation

coderanger
Copy link

This prevents issues in threaded launchers because sys.path is a global value so changing it on one thread changes it for all. This leads to race conditions.

I've been running it for a week now and things seem to be much more stable. There isn't a threaded launcher in Dagster itself, so this should have no impact for normal users.

This prevents issues in threaded launchers because sys.path is a global value so changing it on one thread changes it for all. This leads to race conditions.
@mgasner mgasner requested a review from prha January 4, 2021 23:16
@mgasner
Copy link
Contributor

mgasner commented Jan 4, 2021

I wonder if this will interfere with code pointer loading for certain types of code pointers in a potential threaded implementation @prha

@coderanger
Copy link
Author

It means you need to be more rigorous with module paths but I think that's a reasonable tradeoff for the overhead benefits of a threaded launcher in my case :)

@coderanger
Copy link
Author

I could also make this use an env var instead of checking the thread ID, since this seems to be breaking some of the tests.

@coderanger
Copy link
Author

coderanger commented Jan 5, 2021

It would appear that pytest runs things in a background thread itself, so that's triggering the new behavior :) Thoughts on making this an env var instead? That's unlike other config but there's not really a good way to get config data down this deep in the system without big changes.

Alternatively I could leave the current thread ID check and add an env var to override it, and set that in the test env.

@prha
Copy link
Member

prha commented Jan 29, 2021

I think having the alter_sys_path behavior disable by a very descriptive environment variable seems like the way to go.

I'm curious about the threaded launcher you have. Are you loading repository code to execute in the run launcher process?

Should also note that we use alter_sys_path to load repositories in the workspace that specify a working_directory argument (https://docs.dagster.io/overview/repositories-workspaces/workspaces), which means that your run launcher would not be able to load certain workspaces.

@sryza
Copy link
Contributor

sryza commented Sep 30, 2021

I'm going to close this, because it hasn't made progress in a few months. @coderanger - feel free to reopen if you'd like to pick this back up.

@sryza sryza closed this Sep 30, 2021
@coderanger
Copy link
Author

Sorry about this, got pulled onto product stuff with the new year :-( Will see how things go with our next big upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants