orthanc-server / orthanc-builder

Repo used to build osimis/orthanc docker images, windows installers and OSX packages
GNU Affero General Public License v3.0
29 stars 12 forks source link

Orthanc.pyi inconsistent with actual python plugin types #21

Closed dscarmo closed 1 month ago

dscarmo commented 1 month ago

Hello,

I am using image: orthancteam/orthanc:24.7.3.

It says on Docker Hub that it contains the orthanc python interface version 4.3.

I downloaded the interface file from https://orthanc.uclouvain.be/downloads/cross-platform/orthanc-python/4.3/index.html to better understand the underlying classes and types and provide type annotations on VSCode.

My issue is that the type of class orthanc.ChangeType is not an Enum when checking through running the script, even though orthanc.pyi says it is.

I can't use common Enum methods and attributes.

When checking the type of the object in my script with:

    obj = orthanc.ChangeType(int(changeType))
    print(type(obj))
    print(dir(obj))
    print(obj.__class__)
    print(obj.__class__.__bases__)
    print(isinstance(obj, enum.Enum))

I get:

image

Indicating ChangeType is just a class with class attributes for each type, not an Enum.

This raises concerns if the .pyi file is inconsistent with the current state of the Python Plugin, and if I should trust it.

Can you help me with this? Thanks

amazy commented 1 month ago

Hi,

This .pyi is still very new and we actually do not use it ourselves. Feel free to provide us with a modified .pyi file and we will try to adapt the code that generates it - since it is auto-generated.

jodogne commented 1 month ago

Dear @dscarmo,

First of all, please note that this GitHub repository hosts the builders of Docker images and Windows installers of Orthanc. As your question is unrelated to packaging, you should use our official Discourse forum to discuss this topic.

Please also could you explain how you use the orthanc.pyi in a way that makes it invalid for static type checking and/or auto-completion? As far as I'm concerned, I can see that auto-completion works fine with VSCode and PyCharm. This would allow us to independently reproduce your issue.

As far as the technical details are concerned, all the enumerations in the orthanc package are created by calls to PyDict_New() in the libpython library. Here is the direct link to the source code. So, I agree that the resulting class does not derive from enum.Enum, even though it has the same semantics from an user standpoint. Furthermore, nobody has encountered any issue with this way of creating enumerations over the four years that the Python plugin for Orthanc has been available.

Consequently, from your message, I guess that the simple patch would consist in replacing:

class ChangeType(enum.Enum):

by:

class ChangeType():

Please would you kindly confirm that this would be a solution to your issue?

Another option would be to enforce the use of enum.Enum, as explained in this StackOverflow discussion. As you can see, this process for registering an enumeration is much more complex and could imply compatibility issues. At a first glance, I would not be in favor of implementing it.

Best Regards, Sébastien-

dscarmo commented 1 month ago

Thanks for the detailed answer!

My use case was very simple, and its not a critical problem. I just became suspicious of the interface. Given that it was downloaded from a separate website, I thought I might have had a different/inconsistent interface file, since the types were wrong.

My use case was simply logging every change that occurred in Orthanc for debugging purposes, I was trying to get the corresponding string for the integers that are received by the registered callback. Initially, I tried to use the expected enum.Enum.name method, but it is not implemented since the ChangeType doesn't really derive from Enum.

Example:

class Color(enum.Enum):
    RED = 0
    GREEN = 1
    BLUE = 2

In this class Color(0).name should return "RED".

In the Python plugin, orthanc.ChangeType(0).name does not work as suggested by the enum.Enum father class for ChangeType in the .pyi file.

My workaround looks very ugly. Since we don't have the enum.Enum method, I did this:

class OnChangeCallback():
    CHANGE_TYPE = {getattr(orthanc.ChangeType, attr): attr for attr in dir(orthanc.ChangeType) if not attr.startswith("_")}
    def __call__(self,  changeType: int, level: int, resourceId: str) -> None:
        change_type = OnChangeCallback.CHANGE_TYPE[changeType]
        print(f"Change detected: {change_type}")
        print(f"Level: {level}")
        print(f"Resource ID: {resourceId}")

As I mentioned before, this is a very minor issue but raised suspicion from me if I was using the correct .pyi.

I agree changing the type to just being an object solves this issue. But since the .pyi is auto-generated, I am not sure if this is hiding some bigger issue.

Thanks a lot for answering here even when I created the issue on the wrong place.

jodogne commented 1 month ago

Thanks for your feedback!

As discussed above, the inheritance from enum.Enum has just been removed by the following changeset in the mainline of the Python plugin: https://orthanc.uclouvain.be/hg/orthanc-python/rev/9363da60c3c3

You can find the new version of the interface on the official download site of the Orthanc project, in the mainline folder.

This patch will be included in future releases of the Python plugin.

Best Regards, Sébastien-