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

Expand workflow metadata for readme. #19591

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

jmchilton
Copy link
Member

@jmchilton jmchilton commented Feb 11, 2025

So far just the first part of #19559.

Added help and readme field. Also added a logo url so we can generate workflow repository readmes using a template like

# {{ workflow.name }}

{% if workflow.logo_url  %}
![{{ workflow.name }} logo]({{ workflow.logo_url }})
{% endif %}

{{ workflow.readme }}

I've also renamed the workflow annotation in the UI to "short description". I think this is more clear in light of the new metadata fields. Previously I've seen annotation used for all three purposes - I hope this cleans that up some and makes the workflows more uniform in the workflow index for instance.

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.

Comment on lines +7883 to +7888
readme: Mapped[Optional[str]] = mapped_column(TEXT)
logo_url: Mapped[Optional[str]] = mapped_column(TEXT)
help: Mapped[Optional[str]] = mapped_column(TEXT)
Copy link
Member

Choose a reason for hiding this comment

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

What is the relationship going to be between readme, help and annotation ? And we should probably limit the text size. The largest readme we have in the IWC so far is 3715 characters. Maybe we could start with a 50K char limit in the database and limit this in a setter to 20K chars ? That way we could bump it up without a migration if there are legitimate reasons to have such a long readme ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would describe it as - readme is a full description of the workflow (WHAT), the help is what we place on the form and it is the in-galaxy description of how to use the workflow (HOW), and the annotation is just a short description for use within the workflow list. Do these three make sense to you? The corresponding things for tools would be like a readme in the tool's repository, the help section for the tool, and the description field. The corresponding things for some piece of software like Galaxy would be the readme, the sphinx help, and the description field in Github. It is a very technical and CS perspective I think that the readme and help should be different - I feel strongly about it but also I'm not a user and I would defer to people who actually use Galaxy if they thought the distinction between a readme and help text or an instruction manual would be wasteful.

I've seen some really intricate workflow help text in the annotation and I think it would be great to migrate it to a markdown field.

I will update this to limit the length to 20K characters. The other way to save database space might be to have an association here and update the link only if the text changes. The database might already do this somehow if it is really smart but I don't know if it is or if there is a way to encourage that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Starting to think about the UI I came up with - the readme is text "researchers looking for your workflow will see" and the help is text "researchers running your workflow will see".

Copy link
Member

Choose a reason for hiding this comment

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

The other way to save database space might be to have an association here and update the link only if the text changes

As far as I remember, for both TEXT and VARCHAR (without specifying length), if the field is empty, it will only take space needed for a pointer. So just by declaring a field as TEXT, we're not wasting space.

Maybe we could start with a 50K char limit in the database and limit this in a setter to 20K chars ?

But do we even need a limit in the database? If we control that programmatically in a setter, that'll be the only place we need to edit if we choose to make it 51K.

@jmchilton jmchilton force-pushed the workflow_metadata branch 2 times, most recently from 286c2ee to 0bc9299 Compare February 12, 2025 16:09
@jmchilton
Copy link
Member Author

Most naive possible UI - definitely could use some love.

Screenshot 2025-02-12 at 11 09 48 AM

@jmchilton jmchilton changed the title Backend for workflow readme metadata. Expand workflow metadata for readme. Feb 13, 2025
@jmchilton
Copy link
Member Author

Made some more progress toward a clear delineation between workflow annotations and the workflow readme. I've added a workflow best practice that describes the purpose of both if the annotation is long and encourages moving excess content to the readme or help sections for the workflow.


Screenshot 2025-02-13 at 10 39 07 AM
Screenshot 2025-02-13 at 10 39 46 AM

@jmchilton
Copy link
Member Author

Also added a little readme preview icon to the attributes page that demonstrates I think how we should render the readme content for the repositories and the IWC page.

Screenshot 2025-02-13 at 11 19 37 AM

@@ -7880,6 +7883,9 @@ class Workflow(Base, Dictifiable, RepresentById):
creator_metadata: Mapped[Optional[List[Dict[str, Any]]]] = mapped_column(JSONType)
license: Mapped[Optional[str]] = mapped_column(TEXT)
source_metadata: Mapped[Optional[Dict[str, str]]] = mapped_column(JSONType)
readme: Mapped[Optional[str]] = mapped_column(TEXT)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Text instead of TEXT? I know we mostly use the latter, but, I suspect, there's no reason for that. Whereas the camelcase types are the more flexible, database-agnostic types and are recommended in the SA docs (ref), whereas the uppercase are the exact type.

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.

3 participants