update pyyaml dependency

[ltalirz]

pyyaml 5.1.2 is outdated and, on conda-forge, no build for python 3.9 is available (thus preventing AiiDA from being installed in a python 3.9 conda environment).

see previous discussion in aiidateam/aiida-core#4730 (comment)
and aiidateam/aiida-core#4730 (comment)

[ltalirz]

@sphuber in the second linked comment, you already ran the plumpy tests with pyyaml 5.2 (but the logs are no longer available).

would you be able to restart them and have a quick look to see whether you can get an idea of the issue?

[ltalirz]

@chrisjsewell just saw aiidateam/aiida-core#4730 (review) - did you end up having a look at it? do you have an idea of what the problem might be?

[sphuber]

would you be able to restart them and have a quick look to see whether you can get an idea of the issue?

https://github.com/aiidateam/plumpy/actions/runs/1097403034

[csadorf]

@sphuber Looks like the actions are in a runaway situation? The runs have exceeded 4 hours of runtime now.

[sphuber]

Yeah, as I indicated in the comment that @ltalirz linked, the problem with updating pyyaml is that the engine hangs somewhere when running processes. So it is kind of tricky to debug, which is why I haven't come around to it yet.

[ltalirz]

Ok, thanks @sphuber ! I think this will require someone to run the plumpy tests locally to figure out which test is hanging and where. Any takers @muhrin @sphuber @chrisjsewell ?

[csadorf]

@ltalirz I will have a look.

[csadorf]

@sphuber Please have a look at commit eb7e647 . It should give us some clue as to what is causing this issue.

[sphuber]

@csadorf seems the tests are still hanging. What made you think that this was the culprit?

[csadorf]

@csadorf seems the tests are still hanging. What made you think that this was the culprit?

Odd, this change made the tests pass for me locally.

[ltalirz]

in case it helps, here is the pyyaml changelog

https://github.com/yaml/pyyaml/blob/ee37f4653c08fc07aecff69cfd92848e6b1a540e/CHANGES#L41

If I remember well, the issue first appeared with pyyaml 5.2 (@sphuber correct me if I'm wrong), i.e. perhaps it would make sense to start with testing that upgrade instead of going to 5.4 immediately.

If I'm reading the changelog correctly, then most of the changes are actually trying to correct bugs/inconsistencies introduced in 5.1

Since it updates the default loader in a couple of other places (add_constructor, add_implicit_resolver, add_path_resolver), one thing I could imagine is that perhaps we are relying on the fact that the default loader is still the old one for some of those (?) Just an idea.

P.S. If the issue is indeed machine-dependent (@csadorf are you sure?), it could be somehow related to this "fix for systems with sys.maxunicode <= 0xffff"

[csadorf]

@ltalirz Indeed, I did not realize that at the time I had tested locally with version 5.2. I've now constrained the dependency to '<5.3' in my test branch which should now allow the tests to complete.

@sphuber Can you have another look at https://github.com/aiidateam/plumpy/compare/fix/pyyaml-dependency-csadorf , please? The mypy check fails for some reason that I had not time to investigate yet.

[csadorf]

Tests do indeed pass now: https://github.com/aiidateam/plumpy/runs/3288571087

[ltalirz]

Great! I see you also changed the default loader in one place. Do you still need to comment out the state line?

I'm wondering where the test is actually hanging; I guess it's not in the get_status_info function itself but at some later point (?)
Does plumpy get confused by a weird state?

[csadorf]

Great! I see you also changed the default loader in one place. Do you still need to comment out the state line?

I'm wondering where the test is actually hanging; I guess it's not in the get_status_info function itself but at some later point (?)
Does plumpy get confused by a weird state?

No, the line needs to be commented out. It took me some time to pinpoint this down to that line and I figured that an experienced plumpy developer might be able to provide me with an idea of what might be going wrong based on that information.

[sphuber]

So to summarize, when using pyyaml==5.2 and commenting out the state attribute from the get_status_info, the tests pass? The mypy issue should be easily fixable, simply using the explicit import path instead of the bubbled up one (this is a known issue).

Still, we need to be able to upgrade to pyyaml>=5.4 to solve the critical security issues, don't we?

[ltalirz]

Still, we need to be able to upgrade to pyyaml>=5.4 to solve the critical security issues, don't we?

Yes (and also for the availability of the python 3.9 build on conda, which is not present for pyyaml 5.2).

Still, good to go step by step.

[sphuber]

Still, good to go step by step

Sure. But to answer @csadorf 's question the particular line that you had to comment out to make the tests run doesn't give me any hints. Still not sure what the problem could be.

[sphuber]

The problem with the get_status_info method is that it returns the status of the Process which is an instance of the ProcessState enum. This data needs to be JSON-serialized though when it gets send over RabbitMQ as a response, and it fails there, causing the test to hang because it is indefinitely waiting for a response that never is arriving.

One solution is to serialize the value before returning it, but since it is an enum, we might as well just call str(state) on it and return the string value of the state. This is actually already done in the state_info key, which calls str(self._state) which contains the exact same info. So I propose we remove state_info and just stringify the state value. This solves the issue also for pyyaml>=5.4.

However, for that version, there is another test that hangs test/rmq/test_communicator:test_launch_nowait. Still need to figure out why this hangs.

[csadorf]

@sphuber Feel free to push directly to my branch if you like.