bolkedebruin commented on issue #52753: URL: https://github.com/apache/airflow/issues/52753#issuecomment-3105867091
> Hi [@bolkedebruin](https://github.com/bolkedebruin) , thanks for the feedback. > > > I do not like to check for allowed_deserialize_classes here at all. It has the same concerns we discussed before [@sjyangkevin](https://github.com/sjyangkevin) as it circumvents the security check in the main code. > > The `allowed_deserialize_classes` is the type(s) generated from the `deserializers` list, which doesn’t take anything from the main code. It can be rewritten in a chain of if elif control flow for checking the `cls: type` passed from the main code is actually an identified deserializable object. Let me know if I understand your concern correctly or it’s still a concern related to `import_string`. > I think serializers should not have import_string present. As mentioned this can be done differently, which solves the issue that you see with "Version 1". > > 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 think it would align with “version 1” implementation, both `np.bool` and `np.bool_` are present in the serializers and deserializers list. This can resolve the issue at runtime (if numpy v1 is installed, then it detects np.bool_; if numpy v2 is installed, then it detects no.bool) but failed the test (static check). In numpy v1, the np.bool is considered deprecated and cannot be imported, or invoked directly. In v2, np.bool_ is considered an alias as np.bool and both are valid. > > The current issue is because in the serializers we have only `np.bool_`. It works good when numpy version is 1.x. The whitelist checking in the main code `serde.py` take the qualified name of the object and check against the `serializers` list. In numpy v2, if that is installed in the airflow environment. The qualified name of np.bool or np.bool_ is resolved to `np.bool`, which is not in the serializers list. So, the object cannot match to any serializer and result in the error. I don't follow you here. The list is a set of strings. So adding v2 bool (let's call it bool2) to this list does/should not result in an error by itself. If you then in the code do `hasattr("bool", np)` and act accordingly you will be fine. So: ``` # # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file # distributed with this work for additional information # regarding copyright ownership. The ASF licenses this file # to you under the Apache License, Version 2.0 (the # "License"); you may not use this file except in compliance # with the License. You may obtain a copy of the License at # # http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, # software distributed under the License is distributed on an # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. from __future__ import annotations from typing import TYPE_CHECKING, Any from airflow.utils.module_loading import qualname # lazy loading for performance reasons serializers = [ "numpy.int8", "numpy.int16", "numpy.int32", "numpy.int64", "numpy.uint8", "numpy.uint16", "numpy.uint32", "numpy.uint64", "numpy.float64", "numpy.float16", "numpy.complex128", "numpy.complex64", "numpy.bool", "numpy.bool_", ] if TYPE_CHECKING: from airflow.serialization.serde import U deserializers = serializers __version__ = 1 def serialize(o: object) -> tuple[U, str, int, bool]: if np is None: return "", "", 0, False name = qualname(o) if isinstance( o, np.int_ | np.intc | np.intp | np.int8 | np.int16 | np.int32 | np.int64 | np.uint8 | np.uint16 | np.uint32 | np.uint64, ): return int(o), name, __version__, True # check on name here to avoid an attribute error if name == "numpy.bool" || name == "numpy.bool_": return bool(o), name, __version__, True if isinstance(o, (np.float16, np.float32, np.float64, np.complex64, np.complex128)): return float(o), name, __version__, True return "", "", 0, False def deserialize(cls: type, version: int, data: str) -> Any: if version > __version__: raise TypeError("serialized version is newer than class version") return cls(data) ``` -- 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]
