feat[next]: use context vars instead of global state in embedded iterator execution #1120
Conversation
|
Opening as a reminder, instead of #896 |
|
I can clean up the code and provide a final version of this prototype without too much effort. @tehrengruber do you also agree with using this implementation strategy to fix the problem? |
|
Yes, given the model itself this is much better than the global variable. |
DropD
added
the
move to main
egparedes
force-pushed
the
col-range-cvars
branch
from
67a3ba2
to
e24fd6c
Compare
egparedes
force-pushed
the
col-range-cvars
branch
from
e24fd6c
to
7499384
Compare
egparedes
removed
the
move to main
egparedes
changed the title
egparedes
changed the title
egparedes
changed the title
|
I've requested a formal review from @tehrengruber because apparently @havogt can't approve it since he's the creator of the PR, but I think both of you should take a look. |
egparedes
changed the title
|
|
||
|
|
||
| def test_column_multithread(): | ||
| def test_func(i: int, output: list) -> None: |
There was a problem hiding this comment.
Is this really a meaningful test? I have no clue how the thread scheduling works in Python, but this might very well never fail even if the column range is implemented incorrectly. If we want this to be meaningful how about a lock that ensure the column construction of the first thread happens after the context setup of a second thread? (If it's not worth the effort I would remove the test altogether to be honest.)
There was a problem hiding this comment.
Ok, I agree and removed the test.
| assert np.array_equal(res.data, a.data + b.data) | ||
| assert res.kstart == 1 | ||
|
|
||
| _run_within_context(test_func) |
There was a problem hiding this comment.
This is fine, but is there a reason to not use a context handler for this? Sure the columns would leak scope, but doesn't really matter right?
There was a problem hiding this comment.
A context handler doesn't really work here because code running inside a context needs to be an actual function (line 36). Setting the values of the context vars before running some lines of code and then restoring their previous values does not test the context vars and it doesn't work in the general case (it will fail when running multiple threads, for example).
Changed:
column_rangeandoffset_providerglobal variables have been converted to context variables and fencil closures are now executed in isolated contexts with properly initialized values, to avoid using global state.