Pytest assert rewrite causes serialzation problem with dill #10845
Comments
|
Hi @potiuk - thanks for the report! It seems like this should be reproducible in a much simpler environment (e.g. just using |
|
Unfortunately, I have no idea how and what causes the problem in other way, without airflow. Just to explain it - I have no idea why the way it is used, triggered the error. So following the best way I could help - I observed the error occurs in this environment, and I prepared an easy way to reproduce the environment, that anyone can do, but working out the other way, starting from scratch and trying to reproduce it without airflow, is - I am afraid - beyond my current skills, because I simply have no idea how asserts rewrite work - unfortunately. Instead I prepared a reproducible example (where It is already very easy to reproduce with the image which shows the problem, and for somoene who knows more about how pytest and assert rewrite works should be much easier to figure out what's going on. You do not need to know what airflow is, - it's just an easily reproducible environment where you can debug and test and see the effect of disabling/enabling assert rewrite. I do not know even where to start if I were to start from scratch, to be honest, because I simply have no idea what triggers the issue. But I know the env with docker can be debugged if you have enough knowledge about pytest, I think. If it is a generic issue with dill, then probably you could prepare such a minimum example wiht far less effort than me, and if it is another reason, then I would have to try to find out what really generates it (I even have not written the test myself, so I am not sure what to do to generate such error). UPDATE: See below - actually, it turned out to be easy and the minimum reproducible example is below |
|
But - I'd really love to help to find/solve the issue and maybe that will help you to attempt to make a fix that you would be able to test in the environment I provided? That might be much faster than trying to reproduce a minimal case if we work together. I think with intelligent guess, I know what happens and even how to fix it, but I lack some more detailed knowledge about pytest internal architecture. But I can make some intelligent guesses. I believe, the issue seem to be be because somewhere in the code, we are trying to serialize the whole module with dill and AssertRewriteHook during test has an open opened capture stream which cannot be serialized and the hook keeps a reference to it, so when dill tries to serialize the module, it fails: This is where it happens:
Proposed solution (If I am right of course):I think the solution (though I do not know how exactly to apply it) is to make AssertionRewriteHook to become a serializable object - by making the Stateful AssertionRewriteHook serializable by following this stateful object semantics from pickle https://docs.python.org/3/library/pickle.html#handling-stateful-objects (i.e. implement _get_state and _set_state methods that would remove the non-serializable state (open files) from the serialized dictionary when saving and restore the files on deserializing. Maybe that will be helpful to attempt it (by someone who knows better the internal architecture of the Hook/Pytest)? |
|
Actually following the approach from #10844 and testing my own assumptions above, I found a minimal reproducible example. Requires pyenv, dill, pytest installed. Here are the minium reproducible steps: pyenv virtualenv 3.9 dill-pytest
pyenv activate dill-pytest
pip install pytest dill
mkdir test
cat >test/test_dill.py <<EOF
import dill
import sys
this_mod = sys.modules[__name__]
def test_serialize():
print(dill.dumps(this_mod))
EOF
pytest test/test_dill.pyProduces (Mac OS M1):
When you repeat with plain asserts, it works fine:
You can see the nicely serialized module with -s:
I hope this one (and the one in #10844) - will be helpful and are easy-enough to be incorporated in your test suite. Let me know, please if you need some more input |
|
And obligatory version dump: |
|
BTW. I am also happy if I could get someone to guide me - if my theory about the serialization seems plausible - on where and how this problem could come from, - i.e. where such open non-serializable capture object is kept, and I might attempt to provide a fix (but for now my knowledge about pytest internals is very low, so it would be super-hard to start - but with some guidance, I might be able to find my ways and make a PR. |
|
Fantastic work! Unfortunately I'm not an expert on this particular code, but hopefully someone else will be able to help out. It's at least clearly either a Pytest interop issue, or (less likely?) a bug in |
|
As far as I can tell dill tries to serialise modules like normal objects, which as far as I understand python modules is categorically wrong |
|
Not really. This a feature of dill that it serializes modules and we are legitimately usng it in Airflow to serialize Python code between virtualenvs. So dill serializing modules is expected behaviour and this behaviour is broken when assert rewriting is used. From https://github.com/uqfoundation/dill/blob/master/README.md
|
|
To me that reads like dill choose to explicitly transfer things that are serialization unsafe in a unsafe way and now there is sudden surprise |
|
I see it differently. Dill promise is that it will allow to serialize modules that do not have any inherent serializability problems. It is true that not all modules will be serializable but many will (and we are relying heavily on that feature in Airflow). The problem i see is that this is classing HeisenBug - enabling testing harness is changing behaviour of tested classes, which should be minimized. Testing harness should not do it in general. This module is - in essence serializable, but enabling assert rewriting makes it non-serializable. So i would classify it as a a problem of the testing harness that can be improved by making the AssertRewritingHoolk either serializable or skipped when serializing. I just think it might improve coverage of cases where Pytest's asset rewriting interferes with tested code (this is very similar case with the other issue I opened about Cassandra - in both cases the harness is less transparent that it can be and this is an improvement opportunity to make it more transparent. |
|
Loaders are practically non serializable state Dill ignoring that implies dill is categorically wrong But we can have the serialization attempt raise a more indicative exception for the hook itself One that makes it abundantly clear that the method of module serialization is unsafe by design and can be "enabled" by disabling the hook Dill is clearly doing something that works by chance and is broken by design, I'm not going to pretend it's a theoretically sound approach But I'm ok with giving people information on how to use this footgun as the airflow ecosystem seems to lack a better mechanism |
|
Just note - it's ot only airflow. Apache Beam for example is heavily relying on dill to distribute serialized code and execute individual methods of the python on the remote end without having to recreate complete virtualenvs with all dependencies in all the workers - this is pretty common use case in the data engineering world where you want have arbitrary dependencies added as imports while serializing and executing individual methods in a distributed environments (that helps in scaling and managing such distributed systems). I am not going to defend dill, but i love to discuss it and find out from them what is the rationale about the decision - and I am happy to open an issue in dill & similarly as I opened it here. Coming from OSS an Apache environment I prefer to make judgments on such things after a discussion and hearing the parties involved to understand the context - my experience tells me that my own judgment my be wrong because i lack important context, and something that i judge as But i am not 100% sure if my assumptions are right. I only 'guessed' that the serialization problem comes from AssertionRewritingHook being set as loader. I do see that when asset rewriting is on, the problem occurs but it goes away when I turn it off. I also noticed (somehow accidentally as i it is not exposed in the docs) that assertion rewriting enabling changes the loader to be the hook. And i also see that the error comes from the fact that somewhere the module being serialized has So my guess is that this is the loader that gets serialized, but i might be wrong about it. I simply do not know pytest internals enough to know what is the root cause, and i was hoping with the minimally reproducible case, pytest maintainers can help to asses it - and once we confirm the mechanism that is going on here - i am happy to bring it to dill as well and ask them what is the reason why they are serializing loaders (if we determine here it is through loaders that we get that issue) - maybe we can all learn a thing or two via the discussion and figure out together how to solve it best. So - i guess my question is - can we confirm the theory i came with has some grounds? Would you be able to confirm it? If so - aI am happy to bring it to dill and see if they maybe propose another solution. But maybe (for example) it is pytest that separately adds the captured output files to the module ? Or maybe it does not have to be added at all and the fact the EncodedFile is added to module is accidental ? I would love to understand it. |
|
Use a editable install of pytest and make the reduce method of the assert rewrite hook/loader raise a type error Extra points if it includes the details I mentioned and a unittest for a pr |
|
I was hoping that providing miniumum reprodsucible environment (which I prepared) was there to help pytest maintainers who are knowledgeable about the architecture to be able to verify the hypothesis I made? Are you asking me to debug pytest to learn about your architecture? |
|
I mean - I probably could do it If I had an infintie amount of time, but Is there anything I can help to leveraage your experience and help to investigate the problem you have other that getting generic advices on how to debug it? |
|
BTW. Here is stackrace after I added init to EncodeFile thowing RuntimeError: And run Not sure if this is helpful - but this is the stacktrace. I do not really understand how you use pluggy intenrally and the architecture, so I am not even sure if this is the usage of EncodedFile that causes the problem, but Is there anything else I can type and provide output of ? Anything else I can type to help ? |
What do you mean by |
|
for both im currently very short on time as paternity leave gets more interesting |
|
I also got my holiday break - but I will come back to it shortly. Thanks for the pointers. Will follow up from here. |
|
assertion rewriting is also causing issues with loading scipy if any of their files match the |
|
There's an elephant in the room which it seems we haven't been talked about yet: Why does However, the assertion rewrite logic then reuses pytest/src/_pytest/assertion/rewrite.py Lines 69 to 72 in 5d58b1f pytest/src/_pytest/assertion/rewrite.py Lines 237 to 243 in 5d58b1f Of course we want test files to benefit from assertion rewrite, but even with the default patterns, if we import |
|
I believe its cause the testing is against the working directory so the paterns apply |
Why is this? Why only in the current directory? What about "common" testing code located in a different directory? This is desired behavior IMO. |
|
Perhaps what's needed rather than having users have to list all code they want tested/rewritten is to have a feature that packages can default-opt-out. Imagine a |
|
That's going to break the world Assert rewrite is opt in |
Because assertion rewriting is intended for assertions in your tests, which typically are in the directory you invoke
You should call
I disagree. As you can see here, assertion rewriting can introduce subtle bugs and environment differences. The last thing you want during testing is to test a different situation than what your code is actually running in. I also don't see a big benefit: If non-test code has asserts, that code already needs to take care of proper output on an assertion failure (e.g. |
Am I reading this correctly as recognition that this is a pytest bug? |
|
I think there are two sides to this:
|
|
thanks, that is clear! The nicest solution to me does seem like a better way to specify "all source python files" than
In SciPy, the problem is roughly:
perhaps this is an intended use-case for But the issue of pytest rewriting external libraries seems more fundamental. |
Hmmm... looks like my suggestion is already supported and doesn't break the world. |
|
This could be more usable. I'm not going to sit there and type out 100+ modules that need to be rewritten. Perhaps this could benefit from pattern matching as well? Or being able to specify entire packages? If you are going to fix the issue and make |
From what I can gather,
What's your usecase for wanting assertion rewriting in "100+ modules"?
From the documentation above: "This function will make sure that this module or all modules inside the package will get their assert statements rewritten."
I don't think that will work, since pytest currently bails out early (and lets the normal Python import system handle an import) if it determines a module shouldn't be rewritten. But with that, it follows that there's no way for it to check how the docstring of a module that shouldn't be rewritten looks like. Maybe there's a case to be made for some sort of config setting telling pytest what to rewrite (but then again, you can just call |
Pytest assert rewrite causes serializing issue with dill - apparently it makes AssertRewriteHook present (including output redirection in the code being serialized by airflow, that gets properly serialised when assert rewriting is disabled.
Context: When we added
python_files = "*.py"to Apache Airflow in order to not accidentally skip some of our tests ( apache/airflow#30315 ) at some point one of our tests :tests.operators.test_python.TestPythonVirtualenvOperator.test_airflow_contextstarted to fail with mysterious error:When debugging with debugger, you can see that what happens is that dill is attempting to serialize AssertionRewrritingHook - and since the hook contains references to some files (captured stdout/stderr) - those cannot be serialized by dill.
This does not happen when assert rewrite is turned off, unfortunately I have not found a good workaround to exclude something wia
PYTEST_DONT_REWRITEcommend , so a I had to separate out the test to run separately from other tests with--assert=plainas workaround.Rewrite is the most likely reason because either adding
--assert=plainsolves the problem.Interesting clue. I could also fix the problem by adding exclusion via
python_files. Initially we hadpython_files = ["test_*.py"]and the problem did not appear. The problem started to appear when we changed it topython_files = ["*.py"]. I performed a trial-and-error bisecting on the name that causes the problem and it seems that it isyaml.pythat causes problem (I tried to addPYTEST_DONT_REWRITEto theyaml.pyfiles we have in the system and it does not solve the problem). Reproduction steps showing that are also added.I've opened a PR to cassandra to include PYTEST_DONT_REWRITE datastax/python-driver#1142 and in Apache Airflow we have PR to autoamaticallly patch cassandra driver with it apache/airflow#30315, but those are merely workarounds for the problem.
Reproduction:
An easy way to reproduce it:
We have the test currently skipped so this results in:
PYTEST_PLAIN_ASSERTS=trueThis test fails with long exception and stack trace :
If you debug it - you will see that
AssertionRewritingHookis the object contributing the file to serialize by dill.Result - the test succeeds:
Why in some way 'yaml.py` is a problem?
pyproject.toml(in current working dir) to change:into
It will fail again with
TypeError: cannot pickle 'EncodedFile' objectpyproject.tomlto anything else thanyaml.py- for examplet*.py:I tried to bisect the name -> it continues to fail as you add
y*.py,ya*.pyetc - up toyaml.py. Seems that INCLUDINGyaml.pyin the list of thepython_filestriggers the error.Mandatory information:
Versions
The output of pip list:
pip listfrom the virtual environment you are usingThe text was updated successfully, but these errors were encountered: