3353 normalize start method spawn seems to ignore environment variables #3463
Conversation
djudjuu
force-pushed
the
3353-normalize-start_method-spawn-seems-to-ignore-environment-variables
branch
from
e19526d to
0d50b31
Compare
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 04ca239 | Commit Preview URL | Dec 18 2025, 09:27 AM |
djudjuu
force-pushed
the
3353-normalize-start_method-spawn-seems-to-ignore-environment-variables
branch
from
0d50b31 to
85aa0f5
Compare
| @@ -0,0 +1,143 @@ | |||
| """Test that ConfigSectionContext is properly restored in spawned worker processes.""" | |||
There was a problem hiding this comment.
do we want this test? I wanted to have something that is closer to the actual change, whereas the other test is more of a regression test that fixes the exact issue
rudolfix
left a comment
rudolfix
left a comment
There was a problem hiding this comment.
code is good. tests are missing a case and some of them are not needed
| ), | ||
| ids=lambda x: x.name, | ||
| ) | ||
| def test_normalize_compression_with_spawn_workers( |
There was a problem hiding this comment.
cool test! but running for all destinations is an overkill. pls. move it to tests/pipeline, it does need load step.
|
|
||
| # Check that normalized files are not compressed | ||
| load_storage = p._get_load_storage() | ||
| normalized_packages = load_storage.list_normalized_packages() |
There was a problem hiding this comment.
this method is already on the pipeline
|
|
||
|
|
||
| @configspec | ||
| class SectionedTestConfig(BaseConfiguration): |
There was a problem hiding this comment.
makes sense to have those tests. pls. move them to test_runners.py. (tbh. worker init tests were missing altogether)
| return config.test_value, section_ctx.sections | ||
|
|
||
|
|
||
| def test_config_section_context_restored_in_spawn_worker() -> None: |
There was a problem hiding this comment.
actually sections should be visible both in case of spawn and fork. so test both (parametrize)
|
|
||
| # Store it in container | ||
| container = Container() | ||
| container[ConfigSectionContext] = section_context |
There was a problem hiding this comment.
we are also missing the case when there's not section context in container (do del on container[ConfigSectionContext]`)
you can parametrize this test to chekc that
|
all failing tests are : |
chatgpt-5 so maybe we set the pytest expectation for fork-method to fail and do a follow up issue? are people running dlt-pipelines in github-actions? those would be affected... |
rudolfix
left a comment
rudolfix
left a comment
There was a problem hiding this comment.
looking at failed tests:
forkis not supported on windows. what's weird that creation of process pool is not failing. instead something else happens that skips passing the section context to your test. maybe somehowspawnis used? then obviously no context is passed (you think we do fork but system does spawn)
if start_method != "fork":
ctx = Container()[PluggableRunContext]
you can see what happens if multiprocessing.get_context(method=start_method), force=True is passed
- another thing for fork:
if you start the pool BEFORE adding section to container then obviously you get empty container in the fork - because the process forks when the pool starts
3, on mac the default method is spawn. so I think problem is same as windows. for some reason the start type is not really set
you need to debug carefully how pool is created
djudjuu
force-pushed
the
3353-normalize-start_method-spawn-seems-to-ignore-environment-variables
branch
from
58a797a to
55cbf17
Compare
|
@djudjuu this looks good now. you can remove explicit start method from failing tests. |
djudjuu
force-pushed
the
3353-normalize-start_method-spawn-seems-to-ignore-environment-variables
branch
from
00b7e85 to
04ca239
Compare
Uh oh!
There was an error while loading. Please reload this page.