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

Handle Uuid FullLoader with yaml de/serializer #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Jan 30, 2025

In #221, the yaml loaders are changed to UnsafeLoader to solve the problem after new (at that time it is new, it has been three years now) PyYAML, by default it won't de/se customize type.

The changes in #221 touched two things, one was the deserializer for bundle type which the change is needed, as detail discussion in aiidateam/aiida-core#3709 where issue was originally manifested.

But the change of using UnsafeLoader also made for the kiwipy message decoder, which is not necessary. The reason that the rmq.test_communications:test_launch_nowait test was hanging is that the response ack message from channel is a PID which has type uuid.UUID. It is not a basic type that covered by yaml decoder.

The more fine tuning change I think should be just adding the UUID representer and constructor explicitly.

Here I also explicitly use UnsafeLoader instead of Loader which are the same, but Loader is for backward compatibility and UnsafeLoader is the new API and give the information that it is unsafe operation.

@superstar54 @giovannipizzi, for the workgraph the unsafe_deserializer is what wrapped for the similar purpose I guess. So I'll let you think about whether you can do the same thing by adding all customize types into the range for the yaml deserializer to avoid unsafe keyword. I think for an API exposed to end user, it should never "unsafe" in terms of data/computer security, but the "unsafe" is left for developers to find out where in the source code that cause memory issue or potential security issue.

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.

1 participant