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

EDAM tool panel #10592

Merged
merged 10 commits into from
Jan 8, 2021
Merged

EDAM tool panel #10592

merged 10 commits into from
Jan 8, 2021

Conversation

hexylena
Copy link
Member

@hexylena hexylena commented Oct 30, 2020

part of #10587. This was honestly pretty easy and it looks great? (Minus the hundreds of unannotated tools.)

image

Original Notes

I just took the existing ITP loading code and replaced it with edam generated topics rather than whatever section they requested.

Sorting based on EDAM would be nice (edit: done!) E.g. the following image:

Bugs:

  • we have a copy of the EDAM ontology in the codebase. Surely there's a better way to handle this?
  • Binary server setting, doesn't permit some of the fancy things people wanted in Replace tool panel sections with EDAM #10587
  • Search works but "show sections" in search is now broken
  • All the tests fail, even though I made this opt-in?
  • Sorting based on EDAM ordering would probably be better.

@hexylena
Copy link
Member Author

Went ahead an annotated some of my Circos tools and it's fantastic, I love not having to read through the sections to try and figure out where something should go, it's just automatic.

image

@galaxybot galaxybot added this to the 21.01 milestone Oct 30, 2020
@mvdbeek
Copy link
Member

mvdbeek commented Oct 30, 2020

  • All the tests fail, even though I made this opt-in?

It's just the unit tests that mock out a config object in various places. Want me to fix that ?

@hexylena
Copy link
Member Author

It's just the unit tests that mock out a config object in various places. Want me to fix that ?

Sure, that'd be ace!

@mvdbeek
Copy link
Member

mvdbeek commented Oct 30, 2020

  • we have a copy of the EDAM ontology in the codebase. Surely there's a better way to handle this?

Downloading this file now from beta_edam_ontology_url. Can you rebase out config/EDAM_1.23.tsv ?

I just gave it a go, it looks nice!

@hexylena
Copy link
Member Author

I've added section labels, and ordered all of the contents according to the alphabetically sorted. I thought it would be better, but, not sure how I feel about it in the end.

image

@mvdbeek
Copy link
Member

mvdbeek commented Oct 30, 2020

I like that, it is a consistent way to read the panel.

@hexylena
Copy link
Member Author

Hid labels without children, ok, I definitely like that too.

image

@@ -823,7 +1013,10 @@ def quick_load(tool_file, async_load=True):
integrated_elems[key] = tool

if async_load:
self._load_tool_panel()
if self.app.config.enable_beta_edam_toolbox:
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead make this an API parameter and have this more of a dynamic view of the toolbox?

Copy link
Member Author

Choose a reason for hiding this comment

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

That definitely sounds like a better API. Would you have any suggestions for that refactoring? (It's been a while since I've done dev type things) Doing both the edam version and non-edam version parsing, and building two internal toolboxes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just following up here @jmchilton in case you had any opinions on the implementation :)

@dannon
Copy link
Member

dannon commented Nov 9, 2020

I haven't tried this yet, but it aligns a lot with what @juleengraham has been working towards to support multiple user/admin/upstream defined toolsets in the tool panel. Have you followed that work? (I'm also out of date on the status of that, and haven't tried this yet, but I'm trying to catch up and wanted to comment before I forget!)

@hexylena
Copy link
Member Author

Have you followed that work?

That sounds fantastic, I have not heard about it, or followed it at all. Is there a tracking issue somewhere or so?

@juleengraham
Copy link
Contributor

@hexylena Dannon is referring to PR #9996. I actually brought up this EDAM work in our JHU meeting last week and briefly started a discussion about how the two might be related. At the time we weren't really sure what EDAM tags were. I'm sure we're going to talk about it more later today. I know in your original issue, you mention admins wanting to have their own lists and I think that would be a great way to connect these two projects.

Correct me if I'm wrong but EDAM tags seem to be a part of a tool's configuration (so specified by the tool's creator/maintainer?), but more standardised because EDAM comes from a biological background/definition.

@hexylena
Copy link
Member Author

@juleengraham ah neat, I hadn't seen that. Admins having their own organisations of the tool panel was definitely discussed in the original, but I think it was potentially a bit orthogonal to the goals of an EDAM view? (i.e. I didn't have bandwidth to tackle that problem.)

Correct me if I'm wrong but EDAM tags seem to be a part of a tool's configuration (so specified by the tool's creator/maintainer?), but more standardised because EDAM comes from a biological background/definition.

This is exactly correct. Tool developers tag their tools with the functions and topics of the tool. It's not an admin or user controlled feature. But it could definitely fit into the "one of many" views of a tool panel, like it seems you've been tasked with!

@juleengraham
Copy link
Contributor

This is exactly correct. Tool developers tag their tools with the functions and topics of the tool. It's not an admin or user controlled feature. But it could definitely fit into the "one of many" views of a tool panel, like it seems you've been tasked with!

@hexylena noted! The toolsets weren't necessarily intended to be based on EDAM tags but I could see that being a use case. I started out working based on the EU's pre-curated sets (https://github.com/usegalaxy-eu/usegalaxy-eu-tools) but I've changed the implementation a bit in hopes that it'll be more user-friendly for the admins. I think for now, the lists were meant to be created by the admin of the instance and no individual users

@mvdbeek
Copy link
Member

mvdbeek commented Jan 7, 2021

As an optional / dev setup this looks fine to me and certainly is a great incentive for more EDAM. Would you be opposed to merging as is (after resolving conflicts) @jmchilton @dannon @juleengraham ?

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.

Can you resolve conflicts @hexylena ?

@hexylena
Copy link
Member Author

hexylena commented Jan 7, 2021

Yes, absolutely @mvdbeek

@hexylena
Copy link
Member Author

hexylena commented Jan 7, 2021

Resolved.

Copy link
Member

@jmchilton jmchilton left a comment

Choose a reason for hiding this comment

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

Approve after linting and tests are passed. Sorry for dropping the ball on the review of this. I'd like to take a pass at making it a bit more dynamic but this is a cool step forward. Thanks so much!

@hexylena
Copy link
Member Author

hexylena commented Jan 7, 2021 via email

@mvdbeek
Copy link
Member

mvdbeek commented Jan 8, 2021

@ic4f I'm sure this is an easy one for you, what did we need to do for this unit test failure?

____________ test_config_defaults[beta_edam_toolbox_ontology_path] _____________

test_data = TestData(key='beta_edam_toolbox_ontology_path', expected='EDAM.tsv', loaded='/home/circleci/repo/database/EDAM.tsv')

    @pytest.mark.parametrize('test_data', get_config_data(), ids=get_key)
    def test_config_defaults(test_data):
>       assert test_data.expected == test_data.loaded
E       AssertionError: assert 'EDAM.tsv' == '/home/circle...base/EDAM.tsv'
E         - /home/circleci/repo/database/EDAM.tsv
E         + EDAM.tsv

test/unit/config/test_config_values.py:227: AssertionError

@jdavcs
Copy link
Member

jdavcs commented Jan 8, 2021

I'm looking into this - give me a few minutes :-)

@jdavcs
Copy link
Member

jdavcs commented Jan 8, 2021

@hexylena You need to add the following to this dict: https://github.com/galaxyproject/galaxy/blob/release_20.09/test/unit/config/test_config_values.py#L100

'beta_edam_toolbox_ontology_path': self._in_data_dir('EDAM.tsv'), 

It tells the test how to prepare the expected value: it places the 'EDAM.tsv' file inside the data_dir (which is created at test runtime).

Sorry for the lacking docs (it's all in the comments in that test file).

@dannon
Copy link
Member

dannon commented Jan 8, 2021

@mvdbeek Yeah, no opposition here; my comments earlier were just thinking forward to future iterations :)

@mvdbeek
Copy link
Member

mvdbeek commented Jan 8, 2021

Sorry for the lacking docs (it's all in the comments in that test file).

The test could maybe tell us that, so if it fails we know what to do ?

@jdavcs
Copy link
Member

jdavcs commented Jan 8, 2021

@mvdbeek yes (a message can be passed to the test with the rest of the data for cases where such a message makes sense). However, the path generation for the test can be automated: we have the key, we have the schema which contains the path_resolves_to value. I think that would be the better way to do it: then we don't have to require this extra step from anyone who's editing a path option in the schema. (writing accompanying tests is good; having to modify a nontrivial testing module for trivial things like a new config option - less so, I think).

@mvdbeek mvdbeek merged commit af4d041 into galaxyproject:dev Jan 8, 2021
@jmchilton jmchilton mentioned this pull request Aug 6, 2021
3 tasks
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.

8 participants