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

Dynamic options: add data table filter #12941

Merged
merged 37 commits into from
Nov 22, 2024

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Nov 17, 2021

In a recent gitter discussion (https://gitter.im/galaxy-iuc/iuc?at=6190aeff8c019f0d0b93414b) it was asked for a way to have a select list containing only elements that are not yet in a data table.. Et voila .. there is a data table filter.

The test tool shows how it might be used:

  1. the options are dynamically create from a file (from_file also had no test so far)
  2. the filter removes all options that are already in the data table

For the first part I needed to apply a little hack: from_file seems to assume that the file is in tool-data in the Galaxy root. It would be much nicer if the file would be loaded from the dir (or a tool-data dir in this dir) containing the tool. Otherwise we would not meet the use case of the gitter discussion.

I did not think about other possible use cases (e.g. intersections of data tables) and possibly needed attributes...

Ping @mvdbeek @pvanheus

The PR also adds more linting to filters.

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 contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@github-actions github-actions bot added this to the 22.01 milestone Nov 17, 2021
@bernt-matthias bernt-matthias force-pushed the topic/data-table-filter branch from f99c729 to 725f3c2 Compare November 17, 2021 21:19
@bernt-matthias bernt-matthias changed the title add data table filter dynamic options: add data table filter Nov 17, 2021
@bernt-matthias bernt-matthias force-pushed the topic/data-table-filter branch from 725f3c2 to d608ba1 Compare November 18, 2021 09:05
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.

This is really cool, but from_file should not be able to reference files outside of the tool dir.

lib/galaxy/tools/parameters/dynamic_options.py Outdated Show resolved Hide resolved
lib/galaxy/tools/parameters/dynamic_options.py Outdated Show resolved Hide resolved
<inputs>
<param name="dynamic_select" type="select">
<!-- initialize options from file (contains all entries that could be added to the data table) -->
<options from_file="../test/functional/tool-data/test_file.tsv">
Copy link
Member

Choose a reason for hiding this comment

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

That's a security issue and should not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an idea how to specify another dir during tests, eg test/functional/tool-data/?

I guess we can check if os.path.abspath(path) startswith the tool-data dir... could do this in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to do this in another PR, but we shouldn't merge this before we sorted that out.

Copy link
Member

Choose a reason for hiding this comment

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

Also there is a safe_contains function in galaxy.util you can use

Copy link
Contributor Author

@bernt-matthias bernt-matthias Nov 18, 2021

Choose a reason for hiding this comment

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

ae4c401 contains a possible solution for:

Do you have an idea how to specify another dir during tests, eg test/functional/tool-data/?

Any comments?

@bernt-matthias bernt-matthias force-pushed the topic/data-table-filter branch from 8065a08 to 2a47e13 Compare December 21, 2021 10:46
@mvdbeek mvdbeek modified the milestones: 22.01, 22.05 Jan 26, 2022
@mvdbeek
Copy link
Member

mvdbeek commented Jan 26, 2022

I'm bumping this to 22.05, we still need to figure out what's up with #12941 (comment)

@mvdbeek mvdbeek modified the milestones: 22.05, 22.09 May 25, 2022
@dannon dannon modified the milestones: 23.0, 23.1 Dec 8, 2022
@bernt-matthias bernt-matthias force-pushed the topic/data-table-filter branch from 2a47e13 to bce9300 Compare June 6, 2023 08:28
@mvdbeek mvdbeek removed this from the 23.1 milestone Jun 21, 2023
@mvdbeek mvdbeek marked this pull request as draft June 21, 2023 18:30
from_file does not work (relative paths are not possible .. and in the
tests we do not know the abs path)
@@ -490,6 +490,190 @@ def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
)


FILTER_REQUIRED_ATTRIBUTES = {
Copy link
Member

Choose a reason for hiding this comment

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

@jmchilton is this going to conflict with your modeling work ?

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.

Could you split the linting from the table filter work ?

@bernt-matthias
Copy link
Contributor Author

Could you split the linting from the table filter work ?

Separate PR?

@mvdbeek
Copy link
Member

mvdbeek commented Nov 15, 2024

yes

@jdavcs jdavcs modified the milestones: 24.2, 25.0 Nov 20, 2024
@bernt-matthias
Copy link
Contributor Author

Could you split the linting from the table filter work ?

I have a hard time to find the motivation to invest any more time here. It seems that this will just create another PR that will not be merged for years.

@mvdbeek mvdbeek merged commit 363ab53 into galaxyproject:dev Nov 22, 2024
52 of 54 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants