Reduce tool form building frequency

[davelopez]

Mitigates #18848

When we have multiple data inputs in a tool, the ToolForm will request a tool rebuild for each data input in rapid succession (see #18848 (comment)).

This will ensure that only the last request with all the updated data is sent.

Before

After

How to test the changes?

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[mvdbeek]

This doesn't look quite right as a fix. If we've got those datasets in the initial form build we really don't need to fire onUpdate in the first place.

[davelopez]

It is hard to track where the change is coming from. So many components firing change events are involved here, that is the best solution I could come up with without having intimate knowledge about the tool form and tool build process 😞

Maybe @guerler or someone else more familiar with the tool form can have a closer look?

[guerler]

Thanks for looking into this. There was a LastQueue mechanism specifically built to avoid multiple redundant requests to the API. I would consider a regression in that mechanism to be a bug, it could prevent users from effectively using certain tools due to a large number of possibly slow requests.

[mvdbeek]

This is a workaround though, not a fix. If we come in with a defined set of inputs, there's no reason to fire off any requests.

[mvdbeek]

Nothing's really broken here, I would target dev since I could see this having subtle side-effects that we're notoriously bad at testing.

[davelopez]

Nothing's really broken here, I would target dev since I could see this having subtle side-effects that we're notoriously bad at testing.

Good point! Done!

[mvdbeek]

I don't think request volume / debounce tradeoff is worth it.

[mvdbeek]

We cannot use this.onChange here because on initial load there would be no
change in the formData and the onChange event would not be triggered.

Isn't this is a good thing ? What made you decide to revert this ?

[davelopez]

Isn't this is a good thing ? What made you decide to revert this ?

That change was breaking this test:

galaxy/lib/galaxy_test/selenium/test_workflow_editor.py

Line 971 in e9b7abf

def test_insert_input_handling(self):

Unfortunately, the test results are gone after the rebase. For some reason, the event must be fired on creation for this test to pass (maybe the problem is with the test?). In addition, after intensive debugging, the change didn't seem to make any significant difference in reducing the form rebuilding as far as I could tell, so I decided to revert it. I will restore it and we can investigate the test further.

I don't think request volume / debounce tradeoff is worth it.

Why not, we want the most up-to-date request anyway, no? 🤔

Sorry, I'm afraid I don't know how to improve the situation 😞 I know this is just a palliative measure and the real issue is the complex event interaction in these components.

On the other hand, I could keep debugging and investigating with more time since this is no longer scheduled for this release.

[mvdbeek]

Why not, we want the most up-to-date request anyway, no? 🤔

500ms delay is a lot of delay and a high price to pay.

the change didn't seem to make any significant difference in reducing the form rebuilding as far as I could tell, so I decided to revert it.

This is weird, see #18848 (comment).

I could keep debugging and investigating with more time since this is no longer scheduled for this release.

👍 maybe this is a good moment to see if we want to change how data is passed up and down the component hierarchy.