Skip to content

Show pre-populated landing data values in workflow run form #19935

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

Merged

Conversation

ahmedhamidawan
Copy link
Member

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


<template>
<FormExampleDataElement
v-if="props.value.class === 'Collection'"
Copy link
Member

@mvdbeek mvdbeek Mar 31, 2025

Choose a reason for hiding this comment

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

All types of parameters can be specified in a landing request. While you're at it, please don't call it example data, that's just one application.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a really rough draft (especially the types I've added). I was trying to look locally for a schema of some sort for all examples of landing data.

Will polish this a lot more once I get back to this

@ahmedhamidawan ahmedhamidawan changed the title Show pre-populated example data values in workflow run form Show pre-populated landing data values in workflow run form Mar 31, 2025
@ahmedhamidawan
Copy link
Member Author

Hmm interestingly I hadn't looked but a version of this interface exists already (to some extent):

image

This is what you see when you land from BRC Analytics for e.g., I haven't fully investigated why this is different from a workflow landing from the IWC site for example:

image

Will take the existing view into consideration now...

@ahmedhamidawan ahmedhamidawan force-pushed the show_example_form_data_in_run_form branch from c078e73 to dd64b40 Compare April 15, 2025 17:28
@mvdbeek mvdbeek added this to the 25.0 milestone Apr 29, 2025
@mvdbeek mvdbeek moved this to In Progress in BRC development tasks Apr 29, 2025
@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented May 5, 2025

... I haven't fully investigated why this is different from a workflow landing...

I mean, I do see that this difference comes from the 2 different workflow landing responses:

BRC Analytics IWC Site
image image

and the front end expects src === "url" and checks the url key; and based on that renders the FormDataUri component.

I understand that the backend from then on (once you run) handles all types of parameters, and hence, when you run (on Test), the workflow runs for either workflow landing structure.

But what confuses me is that when I run something of the second (IWC) type response locally, I get this:

image

which comes from here:

if "src" not in input_dict:
raise exceptions.RequestParameterInvalidException(
f"Not input source type defined for input '{input_dict}'."
)

Can someone explain where in the backend we actually accept all types of request states (especially for collections)?

@mvdbeek
Copy link
Member

mvdbeek commented May 5, 2025

It's cause that's not merged yet, #20004

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented May 5, 2025

It's cause that's not merged yet, #20004

Ah ok 😅

I can rebase and see what we can do here...

@ahmedhamidawan ahmedhamidawan force-pushed the show_example_form_data_in_run_form branch from dd64b40 to 32d5ee6 Compare May 6, 2025 00:33
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review May 6, 2025 00:34
@ahmedhamidawan ahmedhamidawan force-pushed the show_example_form_data_in_run_form branch from 32d5ee6 to 809e05b Compare May 7, 2025 15:42
type DataUriFile = BaseDataUri & {
// Must have one of these 2
class?: "File";
class_?: "File"; // alias for `class`
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 definitely not right, class_ isn't public. I would suggest generating the typescript types from the pydantic models.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this confused me on how to type aliases on the client-end.

generating the typescript types from the pydantic models

You mean kind of how the client api schema is built?

Copy link
Member

@mvdbeek mvdbeek May 7, 2025

Choose a reason for hiding this comment

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

Yes, lots of ways to do this. This would be one way:

import json
from galaxy.tool_util_models.parameters import DataOrCollectionRequestAdapter


with open("schema.json", "w") as out:
    out.write(json.dumps(DataOrCollectionRequestAdapter.json_schema()))

then npx --package=json-schema-to-typescript json2ts < schema.json gives you

/* eslint-disable */
/**
 * This file was automatically generated by json-schema-to-typescript.
 * DO NOT MODIFY IT BY HAND. Instead, modify the source JSONSchema file,
 * and run json-schema-to-typescript to regenerate this file.
 */

export type Src = "hda";
export type Id = string;
export type Src1 = "ldda";
export type Id1 = string;
/**
 */
export type Src2 = "ld";
export type Id2 = string;
export type Location = string;
export type Name = string | null;
export type Ext = string;
export type Dbkey = string;
export type Deferred = boolean;
export type CreatedFromBasename = string | null;
export type Info = string | null;
export type Hashes = DatasetHash[] | null;
export type HashFunction = "MD5" | "SHA-1" | "SHA-256" | "SHA-512";
export type HashValue = string;
export type SpaceToTab = boolean;
export type ToPosixLines = boolean;
export type Src3 = "url";
export type Location1 = string;
export type Name1 = string | null;
export type Ext1 = string;
export type Dbkey1 = string;
export type Deferred1 = boolean;
export type CreatedFromBasename1 = string | null;
export type Info1 = string | null;
export type Hashes1 = DatasetHash[] | null;
export type SpaceToTab1 = boolean;
export type ToPosixLines1 = boolean;
export type Class = "File";
export type Src4 = null;
export type Class1 = "Collection";
export type CollectionType = string;
export type Class2 = "Collection";
export type Identifier = string;
export type CollectionType1 = string;
export type Location2 = string;
export type Name2 = string | null;
export type Ext2 = string;
export type Dbkey2 = string;
export type Deferred2 = boolean;
export type CreatedFromBasename2 = string | null;
export type Info2 = string | null;
export type Hashes2 = DatasetHash[] | null;
export type SpaceToTab2 = boolean;
export type ToPosixLines2 = boolean;
export type Class3 = "File";
export type Src5 = null;
export type Identifier1 = string;
export type Elements1 = (CollectionElementCollectionRequestUri | CollectionElementDataRequestUri)[];
export type Elements = (CollectionElementCollectionRequestUri | CollectionElementDataRequestUri)[];
export type Deferred3 = boolean;
export type Name3 = string | null;
export type Src6 = null;
export type Src7 = "hdca";
export type Id3 = string;

export interface DataRequestHda {
  src?: Src;
  id: Id;
}
export interface DataRequestLdda {
  src?: Src1;
  id: Id1;
}
export interface DataRequestLd {
  src: Src2;
  id: Id2;
}
export interface DataRequestUri {
  location: Location;
  name?: Name;
  ext: Ext;
  dbkey?: Dbkey;
  deferred?: Deferred;
  created_from_basename?: CreatedFromBasename;
  info?: Info;
  hashes?: Hashes;
  space_to_tab?: SpaceToTab;
  to_posix_lines?: ToPosixLines;
  src?: Src3;
}
export interface DatasetHash {
  hash_function: HashFunction;
  hash_value: HashValue;
}
export interface FileRequestUri {
  location: Location1;
  name?: Name1;
  ext: Ext1;
  dbkey?: Dbkey1;
  deferred?: Deferred1;
  created_from_basename?: CreatedFromBasename1;
  info?: Info1;
  hashes?: Hashes1;
  space_to_tab?: SpaceToTab1;
  to_posix_lines?: ToPosixLines1;
  class: Class;
  src?: Src4;
}
export interface DataRequestCollectionUri {
  class: Class1;
  collection_type: CollectionType;
  elements: Elements;
  deferred?: Deferred3;
  name?: Name3;
  src?: Src6;
}
export interface CollectionElementCollectionRequestUri {
  class: Class2;
  identifier: Identifier;
  collection_type: CollectionType1;
  elements: Elements1;
}
export interface CollectionElementDataRequestUri {
  location: Location2;
  name?: Name2;
  ext: Ext2;
  dbkey?: Dbkey2;
  deferred?: Deferred2;
  created_from_basename?: CreatedFromBasename2;
  info?: Info2;
  hashes?: Hashes2;
  space_to_tab?: SpaceToTab2;
  to_posix_lines?: ToPosixLines2;
  class: Class3;
  src?: Src5;
  identifier: Identifier1;
}
export interface DataRequestHdca {
  src?: Src7;
  id: Id3;
}

For what you need now I think you can just cross-check against this ? I don't think we need fully auto-generated types here, just the class_ check isn't necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this is good to learn.

So you mean I can keep everything else as is (after cross checking against this and checking if it all matches up) and just remove the check for class_?

Copy link
Member

Choose a reason for hiding this comment

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

There's other inconsistencies as well. It should only be ext, not filetype is what I see directly. https://pypi.org/project/pydantic-to-typescript/ gives a bit more readable output.

export interface BaseDataRequest {
  location: string;
  name?: string | null;
  ext: string;
  dbkey?: string;
  deferred?: boolean;
  created_from_basename?: string | null;
  info?: string | null;
  hashes?: DatasetHash[] | null;
  space_to_tab?: boolean;
  to_posix_lines?: boolean;
}
export interface DatasetHash {
  hash_function: "MD5" | "SHA-1" | "SHA-256" | "SHA-512";
  hash_value: string;
}

export interface CollectionElementCollectionRequestUri {
  class: "Collection";
  identifier: string;
  collection_type: string;
  elements: (CollectionElementCollectionRequestUri | CollectionElementDataRequestUri)[];
}
export interface CollectionElementDataRequestUri {
  location: string;
  name?: string | null;
  ext: string;
  dbkey?: string;
  deferred?: boolean;
  created_from_basename?: string | null;
  info?: string | null;
  hashes?: DatasetHash[] | null;
  space_to_tab?: boolean;
  to_posix_lines?: boolean;
  class: "File";
  identifier: string;
}
export interface DataRequestCollectionUri {
  class: "Collection";
  collection_type: string;
  elements: (CollectionElementCollectionRequestUri | CollectionElementDataRequestUri)[];
  deferred?: boolean;
  name?: string | null;
}

export interface DataRequestUri {
  location: string;
  name?: string | null;
  ext: string;
  dbkey?: string;
  deferred?: boolean;
  created_from_basename?: string | null;
  info?: string | null;
  hashes?: DatasetHash[] | null;
  space_to_tab?: boolean;
  to_posix_lines?: boolean;
  src?: "url";
}
export interface FileRequestUri {
  location: string;
  name?: string | null;
  ext: string;
  dbkey?: string;
  deferred?: boolean;
  created_from_basename?: string | null;
  info?: string | null;
  hashes?: DatasetHash[] | null;
  space_to_tab?: boolean;
  to_posix_lines?: boolean;
  class: "File";
}

are the relevant models I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thank you, I'll replicate this there and adjust them accordingly

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've made the changes here 1913be6 , and also added an explanation in the commit message.

@ahmedhamidawan ahmedhamidawan force-pushed the show_example_form_data_in_run_form branch from 809e05b to 03e2eaf Compare May 7, 2025 21:45
@ahmedhamidawan ahmedhamidawan force-pushed the show_example_form_data_in_run_form branch from 03e2eaf to 7b73979 Compare May 7, 2025 21:47
…ient

As recommended, matched pydantic types. What I mean by "no more type aliases for client" is that if you see the changes in this commit, before this commit, we were allowing several aliases in the workflow landing data `request_state` to be shown in the client components (e.g.: `ext`, `filetype` and `extension` were all valid keys to show the file extension, any of `url` or `location` would work for the source URL, etc).

Now, we have stricter types hence, the workflow landing (for the client workflow run form) requires exactly the expected structure in `src/components/Form/Elements/FormData/types.ts`.

If any landing data differs from this, it is not shown.

Co-authored-by: mvdbeek <[email protected]>
@ahmedhamidawan ahmedhamidawan force-pushed the show_example_form_data_in_run_form branch from 7b73979 to 1913be6 Compare May 7, 2025 21:48
@mvdbeek
Copy link
Member

mvdbeek commented May 8, 2025

Those selenium errors look related ?

@@ -39,6 +39,7 @@ const { steps, loading, loadWorkflowOntoGraph } = useWorkflowRunGraph(
);

try {
datatypesMapperStore.createMapper();
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant with using the store and should have no effect.

@@ -66,6 +67,7 @@ try {
readonly />
</BCard>
</div>
<BAlert v-else variant="danger" show> Unable to load graph due to missing datatypes mapper. </BAlert>
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually reachable ? If it is there must be an actual error message somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

This somehow happened only in the case of workflow landings and trying to load the graph then. Let me take another look and try to find where it originates...

@mvdbeek mvdbeek merged commit f68dcd1 into galaxyproject:dev May 9, 2025
27 of 30 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in BRC development tasks May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants