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

Show readme, changelog and authors in run form for dockstore imported IWC workflows #19536

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Feb 4, 2025

show_dockstore_readme_in_run_form.mp4
simplified_wf_run_READMEs.mp4

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.

@mvdbeek
Copy link
Member

mvdbeek commented Feb 4, 2025

Fetching the changelog would also be pretty nice

@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review February 4, 2025 17:49
@github-actions github-actions bot added this to the 25.0 milestone Feb 4, 2025

// Ensure that the trsID is for a GitHub workflow at iwc-workflows
if (parts[0] !== "#workflow" || parts[1] !== "github.com" || parts[2] !== "iwc-workflows") {
throw new Error("Workflow is not from iwc-workflows");
Copy link
Member Author

Choose a reason for hiding this comment

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

It is fine to throw this, right? Instead of just returning from the function.
One way we could get to this point is if a workflow has source_metadata but the trs_id is not from iwc for example.
I made sure we rethrow the error (simply does a console.debug), and then in WorkflowAnnotation I also catch the error, but do nothing.

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 ok, but you might be hiding real errors. I would use a regular return with empty values in the expected case of it not being an iwc workflow, and then handle the case that something went wrong with the request and show that to the user ? Or log at an error level at least. Always good to discriminate expected things that won't work from unexpected failures that might require a fix.

@ahmedhamidawan ahmedhamidawan changed the title Show README in run form for dockstore imported IWC workflows Show readme, changelog and authors in run form for dockstore imported IWC workflows Feb 4, 2025
@ahmedhamidawan ahmedhamidawan force-pushed the show_dockstore_readme_in_run_form branch from 702f1d7 to 0ce4a2d Compare February 4, 2025 20:36
@ahmedhamidawan ahmedhamidawan force-pushed the show_dockstore_readme_in_run_form branch from 0ce4a2d to e3420fa Compare February 4, 2025 22:11
@bernt-matthias
Copy link
Contributor

Help on top. Nice :)

@lldelisle
Copy link
Contributor

Wouah! Thank you so much

@jmchilton
Copy link
Member

The feature is great and needed and I love it. The implementation is not a good idea IMO - the readme shouldn't be assumed to be the "Help" for the workflow - the workflow should have help/instructions metadata that can optionally just reference a workflow description if the workflow author doesn't want to write separate help instructions. Also the correct place to store this information should be in the workflow not in our Github pipeline. Requiring features of our distribution pipeline to properly render an artifact was such a self own with the toolshed that we are still not recovered from and we should not replicate it here.

Add the metadata you want to the workflow, update the IWC tooling to extract and reuse the metadata how it wants, use it properly in Galaxy based only on the target artifact. The project is 20 years old and this is the most important feature under active develop - we don't need to take shortcuts and we really shouldn't. If we're happy with the UI here - I'd be willing to write the backend pieces to add these artifacts to the workflow itself and to assist with updating the IWC tooling to avoid going down this road.

@ahmedhamidawan
Copy link
Member Author

I made sure that we restricted this to the case where the workflow is most definitely from the IWC (as far as I understand those are the most well maintained ones), and again, as far as I understand, the readme originally comes from that repository, no?

If we have that, why not add an additional (expandable/not in your way) help section which shows it? It's just additional helpful information for the user.

Of course renaming it from Help to README is no big deal.

When you say

Add the metadata you want to the workflow, update the IWC tooling to extract and reuse the metadata how it wants, use it properly in Galaxy based only on the target artifact.

You mean we add the metadata to the workflow object which then references the readme ; but the readme is still stored where?
I do not understand what that means...

I would also love to write the backend pieces which would help do this properly, as opposed to taking this shortcut, and systematically add this information to the workflows in the DB (for the workflows that have TRS ids)

@jmchilton
Copy link
Member

jmchilton commented Feb 6, 2025

I made sure that we restricted this to the case where the workflow is most definitely from the IWC

This is my concern, I get that this was the feature request and you did an excellent job I just don't think it was the right request.

If we have that, why not add an additional (expandable/not in your way) help section which shows it? It's just additional helpful information for the user.

I am happy with the readme being available - it just should be part of the workflow and not dependent on the IWC tooling or publication.

You mean we add the metadata to the workflow object which then references the readme ; but the readme is still stored where? I do not understand what that means...

I've added this #19559 to explain it in detail and recommend an implementation - let me know if it is too vague.

add this information to the workflows in the DB (for the workflows that have TRS ids)

This is I think the mistake we made with tool shed - is is backwards to collect this data from the TRS ID. We implemented features for things that had been published when those features should have been available to tools before publication. You cannot have an image in your tool unless you've published the tool to the tool shed. How do you develop the tool help then? How do you know the publication is going to be correct? What if the tool shed is unavailable for installing the tool in the future?

Having the TRS ID is a link and maybe you can use it for information about how you got the workflow - but the workflow's intrinsic metadata should be editable and available without the TRS ID. Analogous to the tool issues - if I'm developing a workflow and the readme or instructions are going to be included and presented to the user - I think I should be able to see how that is rendered without the publication step so I know I'm publishing the right thing. I also think workflow developers who want to publish their own collections of workflows should have these features available.

It is a lot more work for sure - but I think it is the right thing to do.

@mvdbeek
Copy link
Member

mvdbeek commented Feb 6, 2025

for this PR then I think we could go ahead with showing the resolved ORCIDs and the changelog if we have one ?

@ahmedhamidawan
Copy link
Member Author

for this PR then I think we could go ahead with showing the resolved ORCIDs and the changelog if we have one ?

But the changelog is being retrieved the same way the readmes are? Are we ok with keeping that as is then?

(And if yes, maybe we can also show the readmes for now until we implement builtin readmes (or whatever we wanna call them) in Galaxy?)

@mvdbeek
Copy link
Member

mvdbeek commented Feb 7, 2025

Are we ok with keeping that as is then?

As @jmchilton said:

The CHANGELOG should absolutely be included but we have other issues (xref #9166, #6410) for this and the problem is hard enough I'm happy to have external links to this data for now (i.e. #19536) as long as everyone is on board that we should have a structured way to store and reference this data from the workflow itself longer term.

@ahmedhamidawan
Copy link
Member Author

#19591 should give us what we were looking for...

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