bolkedebruin commented on issue #52753:
URL: https://github.com/apache/airflow/issues/52753#issuecomment-3104164833
@potiuk I might have been opposed making the serializer truly pluggable, I
am not anymore as mentioned in another thread. That being said, here I think we
have a 'compile' time dependency rather than a 'runtime' dependency. Loading of
the serializers is lazy, so task sdk does not have a strong dependency on
pandas, but its tests do as far as I am able to distinguish (I'm less familar
with task sdk).
I think some serializers, like pandas can be moved to the plugin system. If
fast enough, I have some concerns on the overhead but that might be
unwarranted. Others shouldn't be probably, like timezone.
On the implementation here ("Version 2"), I do not think we need to check
for the numpy version and then extend the serializer list. Both can be present
as they are just there for serde to recognize if a particular (de)serializer is
available. For the serializer to encounter a version 2 object but only having a
version 1 numpy would be... weird and for the deserializer it could happen but
is also a strange situation. I suggest here to check for these issues in the
respective functions and raise a TypeError.
Would this solve the error you see @sjyangkevin ?
I *do not* like to check for allowed_deserialize_classes here at all. It has
the same concerns we discussed before @sjyangkevin as it circumvents the
security check in the main code. I'd prefer a pre-commit to guard against this
kind of code.
TLDR; @potiuk yes, using airflow's plugin system is fine and a good idea as
long as it performs. If not taking that route now, I think we can prevent the
error @sjyangkevin by properly implementing what is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
