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

Refactor Tool Panel views structures and combine ToolBox and ToolBoxWorkflow into one component #16739

Merged
merged 12 commits into from
Oct 16, 2023

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Sep 25, 2023

Changed the structure of toolbox received from the backend ToolBox is now an object of ToolSections (and Tools) by id
(sample structure laid out here in #16453 (comment)

Change in backend api/tools return

api/tools?in_panel=True&view=ontology:edam_operations used to return:

[
  {
    "model_class": "ToolSection",
    "id": "operation_0292",
    "name": "Sequence alignment",
    "elems": [ { Tool object }, { Tool object }, ...]
  },
  {
    "model_class": "ToolSection",
    "id": "operation_0491",
    "name": "Pairwise sequence alignment",
    "elems": [ { Tool object }, { Tool object }, ...]
  },
  ...
]

and it now returns:

{
  "ontology:edam_operations": {
      "operation_0292": {
        "model_class": "ToolSection",
        "id": "operation_0292",
        "name": "Sequence alignment",
        "tools": ["tool_id", "tool_id", ...],
      },
      "operation_0491": {
        "model_class": "ToolSection",
        "id": "operation_0491",
        "name": "Pairwise sequence alignment",
        "tools": ["tool_id", "tool_id", ...],
      },
      ...
  }
}

And we use api/tools?in_panel=false to return a toolsById object to reference these views.

Going forward, this will make it easier for us to make any structural changes with these lighter "view" objects that store only tool ids, to then create the Tool Panel views we like at the front end.

Note: Case where a ToolSection contains a ToolSectionLabel

In such cases (a "Sub" Label within a Section):
image
the new api structure will do something like this:

"filter": {
  "model_class": "ToolSection",
  "id": "filter",
  "name": "Filter and Sort",
  "version": "",
  "description": null,
  "links": null,
  "tools": [
    "Filter1",
    "sort1",
    "Grep1",
    {
      "model_class": "ToolSectionLabel",
      "id": "gff",
      "text": "GFF",
      "version": "",
      "description": null,
      "links": null
    },
    "Extract_features1",
    "gff_filter_by_attribute",
    "gff_filter_by_feature_count",
    "gtf_filter_by_attribute_values_list"
  ]
},

Fixes #16499
Fixes #16101

Also consolidated ToolBox and ToolBoxWorkflow into one ToolBox that is contained within a ToolPanel. Removed Tool panel providers and ProviderAware tool boxes

How to test the changes?

(Select all options that apply)

  • 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.

@ahmedhamidawan ahmedhamidawan added kind/enhancement area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes area/tools area/backend labels Sep 25, 2023
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend looks good so far!

lib/galaxy/tool_util/toolbox/base.py Outdated Show resolved Hide resolved
@jmchilton
Copy link
Member

Parts of this seem correct. I certainly like the idea of just sending the backbone of tool sections - that seems like such a conceptual win from an API perspective. We're still implementing multiple different tool panel views and of different types and such though right? The thing that causes some anxiety on my end is we had like turned ontologies into just another tool panel view and now we're like recomputing some ontology code on the client that had shifted to the backend. It might be used in totally fine ways though. Cool stuff though - even if it makes me anxious!

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Sep 26, 2023

John's comment...

Parts of this seem correct. I certainly like the idea of just sending the backbone of tool sections - that seems like such a conceptual win from an API perspective. We're still implementing multiple different tool panel views and of different types and such though right? The thing that causes some anxiety on my end is we had like turned ontologies into just another tool panel view and now we're like recomputing some ontology code on the client that had shifted to the backend. It might be used in totally fine ways though. Cool stuff though - even if it makes me anxious!

Thank you @jmchilton !

We're still implementing multiple different tool panel views and of different types and such though right?

Yes, we're still treating those as the same (i.e.: At the front end, the Tool panel itself appears and works the exact same way.
Only difference is that the structure received from the backend has changed (as i have updated in the PR description)

@ahmedhamidawan ahmedhamidawan changed the title Refactor Tool Panel views into one Toolbox Refactor Tool Panel views structures and combine ToolBox and ToolBoxWorkflow into one component Sep 26, 2023
@jmchilton
Copy link
Member

Thanks for the clarification - I appreciate you easing my anxieties! This is exciting work - very nice job.

@ElectronicBlueberry
Copy link
Member

Regarding this example:

"filter": {
  "model_class": "ToolSection",
  "id": "filter",
  "name": "Filter and Sort",
  "version": "",
  "description": null,
  "links": null,
  "tools": [
    "Filter1",
    "sort1",
    "Grep1",
    "panel_label_gff",       <- this part
    "Extract_features1",
    "gff_filter_by_attribute",
    "gff_filter_by_feature_count",
    "gtf_filter_by_attribute_values_list"
  ],
  "panel_labels": [      <- and this part
    {
      "model_class": "ToolSectionLabel",
      "id": "gff",
      "text": "GFF",
      "version": "",
      "description": null,
      "links": null
    }
  ]
},

If I understand this correctly, this treats strings within the "tools" array which are prefixed with "panel_label" in a special manner, referencing a label defined in "panel_labels".

If this is the case, I do not like the design choice of giving some string special type implications. What I mean by this is that all other strings here are tool identifiers, while "panel_label_gff" is a panel label identifier. This is confusing when working with the API return, and can lead to edge cases if a tool id happens to start with "panel_label_".

I'd prefer the label object to be directly inside the array, next to the IDs. This still gives the array a mixed type, which is not ideal, but now that mixed type is obvious and easier to handle, rather than be hidden behind indirection.

@ahmedhamidawan
Copy link
Member Author

Thanks @ElectronicBlueberry
That makes sense, i also thought about giving all tool ids the prefix tool_ but your point is better.

I did think about placing the ToolPanelLabel in the tools array but was worried about mixed types but i don't think its too bad like you said. I'll make this change, thanks!

@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review September 29, 2023 00:01
@github-actions github-actions bot added this to the 23.2 milestone Sep 29, 2023
@ahmedhamidawan

This comment was marked as resolved.

@ahmedhamidawan ahmedhamidawan force-pushed the toolbox_restructuring branch 2 times, most recently from e048175 to 09c51f1 Compare October 9, 2023 16:12
@dannon dannon closed this Oct 10, 2023
@dannon dannon reopened this Oct 10, 2023
ahmedhamidawan and others added 10 commits October 13, 2023 19:29
Changed the structure of toolbox received from the backend
ToolBox is now an object of ToolSections (and Tools) by id

Also consolidated `ToolBox` and `ToolBoxWorkflow` into one `ToolBox`
that is contained within a `ToolPanel`. Removed Tool panel providers
and ProviderAware tool boxes
improve typing in several places
@dannon dannon merged commit 7c013cc into galaxyproject:dev Oct 16, 2023
43 checks passed
@dannon
Copy link
Member

dannon commented Oct 17, 2023

@ahmedhamidawan and I talked about the API changes a bit and the plan is to adjust this so that the old route doesn't change and will continue to return the same to_dict, supporting to_panel_view instead via an extra query argument (or explicit route under /api/tools/?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/tools area/UI-UX kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
Development

Successfully merging this pull request may close these issues.

Refactor ToolBox and ToolBoxWorkflow into one component Refactor Tool Panel views into one Toolbox
6 participants