search

aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
7 stars 17 forks source link

Make some changes in AWB 2.0 backwards compatible #426

Closed danielhollas closed 1 year ago

danielhollas commented 1 year ago

I've been thinking about how the rest of AiiDAlab apps will need to migrate to our new stack and AiiDA-2.0. From our experience with QeApp and my application we know that it is a bit of a pain, to say the least. Therefore I'd suggest we try to make it easier for others to migrate, otherwise we risk Python2-Python3 style split in the ecosystem.

One concrete suggestion specifically for AWB would be to make certain things backwords compatible. Concretely, some widgets were changed to use UUID in traitlets instead of AiiDA Nodes. That was a neccessary change, but we can easily make it backwards compatible by preserving the original traitlets, and making a small shim which converts them to UUID.

What do you think @yakutovicha @unkcpz. (LMK if my suggestion is not clear).

danielhollas commented 1 year ago

To make this more specific, here's a code snippet to make the ProcessMonitorWidget backwards compatible.

class ProcessMonitor()
    process = traitlets.Instance(ProcessNode, allow_none=True)

    @traitlets.observe("process")
    def _observe_process(self, change):
        # TODO: Perhaps issue a deprecation warning here
        process = change["new"]
        if process is None:
             self.value = None
             return
        self.value = process.uuid

@yakutovicha @unkcpz could you take a brief look at this issue if you have a bit of time? Would be nice to discuss on today's meeting.

yakutovicha commented 1 year ago

I would prefer not to do that, tbh. Since the work is done in awb (which was the largest chunk of the work), migrating the apps should not be a problem. At this point, maintaining the backwards compatibility feels like an additional hurdle.

danielhollas commented 1 year ago

Since the work is done in awb (which was the largest chunk of the work), migrating the apps should not be a problem.

I am not sure we're understanding each other. The problem is we changed the input traitlets for some of the widgets, which actually makes the migration harder for the Apps developers. So to alleviate this burden I propose this workaround, because I feel there's already quite some work to migrate anyway.

unkcpz commented 1 year ago

I think this is what we need to finish before we make the official release. We do this once or if we ignore it now it will haunt there and make widgets not very intuitive to be reused. Maybe I am naive, I think the only thing to do is to rename the traitlets. Then from QeApp and @danielhollas's app we update to be compatible with the new interface. Before the release of AWB 2.0.0 we still have a chance to do it aggressively.

danielhollas commented 1 year ago

Okay, I think we really need to discuss overall strategy / direction in person, since we all seem to be pushing in different directions.

My overall impression was that we were doing too many breaking changes (not only in AWB, but elsewhere), and not all of them were needed. This makes to migration harder for app developers, but also for us as AiiDAlab maintainers (e.g. now we need to port aiidalab fixes to old docker stack, but aiidalab package has some (minor) breaking changes).

Can we set a meeting for sometimes next week? This week there's work to do already, and AWB 2.0 shouldn't be released before the compatibility work in aiidalab and aiidalab-home anyway. If you agree, let's discuss the meeting date on Slack.

unkcpz commented 1 year ago

I list here the widgets that still use the old interface having process a ProcessNode:

Following are process followers and tested by process.ipynb:

Having calculation which is as CalcJobNode

unkcpz commented 1 year ago

@yakutovicha put some effort to having the backwards compatibility in https://github.com/aiidalab/aiidalab-widgets-base/pull/471. It terms out the codes are not very clear and hard to maintain in the future. Since we currently have controls on all apps in the appstore, the backword compatibility is not critical important. But we should proceed with the backward incompatible changes listed here.