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

Issues with soma.web.ControllerWidget #64

Open
4 of 12 tasks
denisri opened this issue Jan 23, 2024 · 21 comments
Open
4 of 12 tasks

Issues with soma.web.ControllerWidget #64

denisri opened this issue Jan 23, 2024 · 21 comments
Labels
6.0 bug Something isn't working

Comments

@denisri
Copy link
Contributor

denisri commented Jan 23, 2024

This ticket is not a single issue but will discuss and try to improve soma.web.ControllerWidget. We can summarize a short problems list here (by editing the ticket) and discuss in answers.

  • First, I had difficulties when opening it on a controller containing None values. I have fixed a few problems by adding tests, and I will push fixes.
  • The ControllerWidget opened on a Morphologist instance displays nothing, without a visible error.
  • The ControllerWidget opened on a Morphologist instance displays all values empty.
  • Deleting the widget (del controller_widget) doesn't actually destroy it: it remains visible. But gc.collect() does delete it afterwards.
  • ControllerWidget lacks the former set_visible() method to hide or show some selected fields
  • Values manually modified in ControllerWidget are not marked as modified as they used to do in Capsul 2. This is required to block completion on manually forced fields.
  • Fields names / values display layout should be adjusted / fixed. For now they cannot be properly resized, and there is no horizontal scrollbar, thus we need to set the widget fullscreen to get a chance to read the value
  • A number of field types are not supported, resulting in the full ControllerWidget to be completely empty.
  • Errors like unsupported types are not displayed nor logged and do not raise an exception, thus debugging is difficult.
  • A Controller view is either read-only, or completely modifiable, that is not only values can be edited, but fields canbe removed etc. We need a 3rd state where only values can be edited, and fields are fixed (except for OpenKeyControllers).
  • We should also be able to specify when expandable fields (like lists, dicts etc) are displayed expanded or folded at the beginning.
  • A few fields metadata may prove useful also, in order to display things to the user in tooltips, colors, or other visual hints: for instance input/ouput state, locked fields, or other information. Thus considering this and earlier points in this list, I guess controller descriptions should be passed in a 2-level JSON dict rather than the "direct" one passed now.
@denisri denisri added bug Something isn't working 6.0 labels Jan 23, 2024
denisri added a commit that referenced this issue Jan 23, 2024
@denisri
Copy link
Contributor Author

denisri commented Jan 23, 2024

How do we debug things in the javascript code ?
There must be a type not handled in Morphologist field somewhere, but I don't know how to figure it out, or print debug messages...

@sapetnioc
Copy link
Contributor

Either you can use a sample code using a http server instead of html in Qt. Or you can trun debugging on for QtWebEngine and use a browser to debug:

https://doc.qt.io/qt-6/qtwebengine-debugging.html

I think the Qt debugging interface in browser doesn't work as well as built-in l browsers one but it is simpler to set up.

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

Is there a sample code somewhere to start a http server displaying a Controller in soma.web ? I'm afraid I can't do it myself...

@sapetnioc
Copy link
Contributor

Yes, sorry I should have tell it before. There is a sample code in web/__main__.py in the web_server_gui function. This is really for debug but this is helpful so it may be a good idea to put it elsewhere (web/debug.py ?).

@sapetnioc
Copy link
Contributor

I forgot to say that the internal server engine exposes all files in static directories of the various projects using it (for now soma-base, capsul and snapcheck) in a static/{filename} route. Therefore, for web_server_gui sample code, the browser entry point is https://localhost:8080/static/controller.html`. It corrseponds to the file web/static/controller.html.

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

Ah OK thanks. I could finally start and use the web backend. But for the Morphologist process, nothing is displayed, no error, nothing in the console. However the Morphologist process instance is actually a Controller (with 228 fields), so I don't know where to investigate.

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

OK this is my example code:

from capsul.api import executable, Capsul
import http, http.server
from soma.web import SomaHTTPHandler, WebBackend

mp = executable('morphologist.capsul.morphologist')

class Handler(SomaHTTPHandler, web_backend=WebBackend(controller=mp)):
    pass

httpd = http.server.HTTPServer(("", 8080), Handler)
httpd.serve_forever()

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

and the response to the controller request:

_json_result	Object { _json_error_type: "TypeError", _json_error_message: "Type not compatible with JSON schema: pydantic.conlist(float, min_items=3, max_items=3)", _json_traceback: 'Traceback (most recent call last):\n File "/casa/host/build/python/soma/web/__init__.py", line 307, in wrapper\n result = callable(*args, **kwargs)\n File "/casa/host/build/python/soma/web/__init__.py", line 351, in get_view\n "schema": json_controller.get_schema(),\n File "/casa/host/build/python/soma/web/__init__.py", line 47, in get_schema\n schema_type = self._build_json_schema(\n File "/casa/host/build/python/soma/web/__init__.py", line 264, in _build_json_schema\n properties[f.name] = cls._build_json_schema(\n File "/casa/host/build/python/soma/web/__init__.py", line 290, in _build_json_schema\n raise TypeError(\nTypeError: Type not compatible with JSON schema: pydantic.conlist(float, min_items=3, max_items=3)\n' }

thus the type pydantic.conlist(float, min_items=3, max_items=3) (list of 3 floats) is not known.

@sapetnioc
Copy link
Contributor

Do I need some specific branch for morphologist ? I have this error:

Traceback (most recent call last):
  File "/home/yann/dev/casaconda/test.py", line 5, in <module>
    mp = executable('morphologist.capsul.morphologist')
  File "/home/yann/dev/casaconda/src/capsul/capsul/application.py", line 343, in executable
    result = executable_from_python(definition, item)
  File "/home/yann/dev/casaconda/src/capsul/capsul/application.py", line 397, in executable_from_python
    result = item(definition=definition)
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/morphologist.py", line 12, in __init__
    super(Morphologist, self).__init__(
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/axon/axonmorphologist.py", line 18, in __init__
    super(AxonMorphologist, self).__init__(False, **kwargs)
  File "/home/yann/dev/casaconda/src/capsul/capsul/pipeline/pipeline.py", line 248, in __init__
    self.pipeline_definition()
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/morphologist.py", line 21, in pipeline_definition
    self.add_process('importation',
  File "/home/yann/dev/casaconda/src/capsul/capsul/pipeline/pipeline.py", line 414, in add_process
    node = executable(process, **kwargs)
  File "/home/yann/dev/casaconda/src/capsul/capsul/application.py", line 343, in executable
    result = executable_from_python(definition, item)
  File "/home/yann/dev/casaconda/src/capsul/capsul/application.py", line 397, in executable_from_python
    result = item(definition=definition)
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/axon/importt1mri.py", line 16, in __init__
    self.add_trait('input', File(allowed_extensions=['.nii.gz', '.svs', '.bmp', '.dcm', '', '.i', '.v', '.fdf', '.mgh', '.mgz', '.gif', '.ima', '.dim', '.ndpi', '.vms', '.vmu', '.jpg',
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/soma/controller/controller.py", line 631, in __getattr__
    raise AttributeError(
AttributeError: <class 'morphologist.capsul.axon.importt1mri.ImportT1MRI'> object has no attribute 'add_trait'

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

You need the branch capsul3 of morphologist-gpl, of course :p

@sapetnioc
Copy link
Contributor

I tried with this branch but then I have this error:

Traceback (most recent call last):
  File "/home/yann/dev/casaconda/test.py", line 5, in <module>
    mp = executable('morphologist.capsul.morphologist')
  File "/home/yann/dev/casaconda/src/capsul/capsul/application.py", line 343, in executable
    result = executable_from_python(definition, item)
  File "/home/yann/dev/casaconda/src/capsul/capsul/application.py", line 397, in executable_from_python
    result = item(definition=definition)
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/morphologist.py", line 32, in __init__
    super(Morphologist, self).__init__(
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/axon/axonmorphologist.py", line 16, in __init__
    super(AxonMorphologist, self).__init__(False, **kwargs)
  File "/home/yann/dev/casaconda/src/capsul/capsul/pipeline/pipeline.py", line 248, in __init__
    self.pipeline_definition()
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/morphologist.py", line 43, in pipeline_definition
    super(Morphologist, self).pipeline_definition()
  File "/home/yann/dev/casaconda/build/lib/python3.10/site-packages/morphologist/capsul/axon/axonmorphologist.py", line 249, in pipeline_definition
    self.add_link(
  File "/home/yann/dev/casaconda/src/capsul/capsul/pipeline/pipeline.py", line 923, in add_link
    ) = self.parse_link(link, check=check)
  File "/home/yann/dev/casaconda/src/capsul/capsul/pipeline/pipeline.py", line 798, in parse_link
    dest_node_name, dest_plug_name, dest_node, dest_plug = self.parse_parameter(
  File "/home/yann/dev/casaconda/src/capsul/capsul/pipeline/pipeline.py", line 877, in parse_parameter
    raise ValueError(
ValueError: 'SulciRecognition' is not a valid parameter name for node 'SulciRecognition' 

@sapetnioc
Copy link
Contributor

By the way, it will be easy to have a quick fix for the error because there is already a series of if to convert specific types to JSON schema. However, a complete fix will requires to see how to add the list size constraint in the schema and to take it into account when creating new value for this type. Otherwise new values will be empty lists.

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

ah yes you also need the branch capsul3 of morpho-deepsulci, sorry.

@sapetnioc
Copy link
Contributor

I made some fixes to avoid failure on unsupported types. There are two things missing:

  • values of fields of type list (without item types) are simply ignored, nothing is inserted in the view for them
  • Length of fixed length lists is not taken into account in view. It may cause problem during item creation but not sure.

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

OK I can see the controller now :)

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

... but values displayed are all empty...

@denisri
Copy link
Contributor Author

denisri commented Jan 24, 2024

The correct json dict of values is actually part of the request answer, and looks OK, but values are not filled in the controller GUI (neither in web nor Qt mode)

@denisri
Copy link
Contributor Author

denisri commented Jan 30, 2024

I looked a bit further into this problem. The key is that a "normal" Controller and a capsul Process don't serialize in json the same way. controller.js receives as value for build_elements_object():

  • for Controller:
    c = Controller()
    c.add_field('toto', str)
    c.toto = 'bububulp'
    
    will be transfered as:
    Object {
      toto: "bububulp"
    }
    
  • for Process:
    class P(Process):
        a: str
    
    p = P('pouf')
    p.a = 'babar'
    
    will become:
    Object {
      definition: "pouf",
      parameters: Object {
        a: "babar",
        activated: true,
        enabled: true,
        node_type: "processing_node"
      }
      type: "process",
      uuid: "d2fe986f-8c90-46da-a2c5-0bdf5650a39b"
    }
    

So a Controller fields are found directly in its top-level json dictionary, whereas for a Process the fields are found in the "parameters" sub-dict.

@denisri
Copy link
Contributor Author

denisri commented Jan 30, 2024

So in controller.js in build_elements_object() if I replace:

        for (const i in sortable) {
            const field_name = sortable[i][0];
            const field_type = sortable[i][1];
            const field_deletable = !this.is_read_only(id) && !field_type.brainvisa.class_field;
            if (value != undefined) {
                const elements = this.build_elements((id ? `${id}/${field_name}` : field_name), field_name, field_deletable, field_type, value[field_name]);

with:

        for (const i in sortable) {
            const field_name = sortable[i][0];
            const field_type = sortable[i][1];
            const field_deletable = !this.is_read_only(id) && !field_type.brainvisa.class_field;
            if (value != undefined) {
                var sub_value;
                if (value.type == 'process' || value.type == 'pipeline')
                    sub_value = value.parameters[field_name];
                else
                    sub_value = value[field_name];
                const elements = this.build_elements((id ? `${id}/${field_name}` : field_name), field_name, field_deletable, field_type, sub_value);

then I can finally see the process controller with correct values !
But this is not really generic, we have to test the controller type to be "process" or "pipeline" (which could extend maybe to "custom_node", or whatever later ?), and a process is not seen as a controller.

@denisri
Copy link
Contributor Author

denisri commented Feb 7, 2024

@sapetnioc has fixed the Controller/Process discrepency in json description: Process instances are now displayed correctly.

@denisri
Copy link
Contributor Author

denisri commented Feb 7, 2024

To demonstrate the layouts issues, here is a little bit of code which displays 2 controllers, one very small (1 field) and one very large, in the same Qt widget. It is using an approximate transcription of the Morphologist pipeline signature, as a standard Controller, via a JSON file:

from soma.qt_gui.qt_backend import Qt
from soma.qt_gui.qt_backend import QtWebEngineWidgets

Qt.QApplication.setAttribute(Qt.Qt.AA_ShareOpenGLContexts)

# %gui qt


from soma.controller import Controller, OpenKeyController
import json
import os
from soma.web import ControllerWidget

osp = os.path


if '__file__' not in globals():
    json_fname = 'morpho.json'
else:
    json_fname = osp.join(osp.dirname(__file__), 'morpho.json')


def json_to_controller(d):
    c = Controller()

    for k, v in d.items():
        if isinstance(v, list):
            dt = list
            if len(v) != 0:
                dt = list[type(v[0])]
            c.add_field(k, type_=dt)
        elif isinstance(v, bool):
            c.add_field(k, type_=bool)
        elif isinstance(v, str):
            c.add_field(k, type_=str)
        elif isinstance(v, int):
            c.add_field(k, type_=int)
        elif isinstance(v, float):
            c.add_field(k, type_=float)
        elif isinstance(v, dict):
            dt = str
            if len(v) != 0:
                dt = type(next(iter(v.values())))
            c.add_field(k, type_=OpenKeyController[dt])
        else:
            raise TypeError('unsupported type:', type(v), 'for field', k)

        setattr(c, k, v)

    return c


def build_widget(smallc, c):
    w = Qt.QWidget()
    lay = Qt.QVBoxLayout()
    w.setLayout(lay)
    cw1 = ControllerWidget(smallc, False)
    lay.addWidget(cw1)
    cw2 = ControllerWidget(c, False)
    lay.addWidget(cw2)
    return w


with open(json_fname) as f:
    d = json.load(f)

c = json_to_controller(d)

smallc = Controller()
smallc.add_field('label', type_=str)

run_loop = False
if Qt.QApplication.instance() is None:
    qapp = Qt.QApplication([])
    run_loop = True

w = build_widget(smallc, c)
w.show()

if run_loop:
    qapp.exec()

which looks like this:
controllerwidget
The JSON file is attached:
morpho.json

As we can see, the fist ControllerWidget gets too much vertical space, and the second takes all the horizontal space for the fields labels, forcing the user to scroll right to see the values, and even there, the values don't get enough space to display their content; no scrollbar allows to see the fields contents, so you just need to have a very wide screen ;)
controllerwidget2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.0 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants