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

Task/WP-211: App Form updates to allow target path #857

Merged
merged 8 commits into from
Sep 29, 2023

Conversation

chandra-tacc
Copy link
Collaborator

@chandra-tacc chandra-tacc commented Sep 13, 2023

Overview

TAPIS V3 provides a mechanism to pass in path to copy input contents into. This path is optional.
From tapis docs:
"A JobFileInput object is complete when its sourceUrl and targetPath are assigned; this provides the minimal information needed to effect a transfer. If only the sourceUrl is set, Jobs will use the simple directory or file name from the URL to automatically assign the targetPath. Specifying a targetPath as “*” results in the same automatic assignment."
TAPIS V3 doc

Related

  • WP-211
  • App Form: Add support for "targetPath" of a fileInput

Changes

  • Add form field for target path when processing file inputs
  • When populating job data, retrieve target path info and populate it according to tapis v3 structure.
  • Use notes: targetPath as a hint to show or hide target Path in App Form. The default is false.

Testing

  1. Check if extract or compress app are in app tray or any other app that takes file input.
  2. Target Path is shown for extract app.
  3. Target Path is NOT shown for compress app. This is hidden based on the app definition (see notes section).
  4. Check that for every field with file input, there is a Target Path for field added to the form
  5. Check with different input types - providing a name, * or empty value (results in *) work with successfully creating a job.
  6. Check if the job output matches what is expected.

UI

Input form change
Screenshot 2023-09-26 at 10 14 55 AM

Input with no target path (example OpenSees)
Screenshot 2023-09-26 at 10 32 37 AM

Successful job submission
Screenshot 2023-09-12 at 6 22 20 PM

Job payload updated
Screenshot 2023-09-12 at 6 28 11 PM

Job output folder
Screenshot 2023-09-12 at 6 29 35 PM

Notes

Screenshot 2023-08-02 at 2 56 46 PM

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #857 (6388cf3) into main (eca948c) will decrease coverage by 0.13%.
The diff coverage is 20.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #857      +/-   ##
==========================================
- Coverage   63.12%   62.99%   -0.13%     
==========================================
  Files         427      427              
  Lines       12148    12188      +40     
  Branches     2492     2504      +12     
==========================================
+ Hits         7668     7678      +10     
- Misses       4275     4303      +28     
- Partials      205      207       +2     
Flag Coverage Δ
javascript 68.71% <20.83%> (-0.29%) ⬇️
unittests 57.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...c/components/Applications/AppForm/AppFormSchema.js 75.00% <33.33%> (-7.36%) ⬇️
...rc/components/Applications/AppForm/AppFormUtils.js 50.74% <42.85%> (-2.09%) ⬇️
...nt/src/components/Applications/AppForm/AppForm.jsx 64.78% <4.00%> (-4.71%) ⬇️

@chandra-tacc chandra-tacc marked this pull request as ready for review September 13, 2023 15:06
Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

Looks good! Just once thing I found when a sourceUrl is not defined, for example with the app https://cep.test/workbench/applications/hello-world?appVersion=0.0.1

client/src/components/Applications/AppForm/AppForm.jsx Outdated Show resolved Hide resolved
@chandra-tacc
Copy link
Collaborator Author

Looks good! Just once thing I found when a sourceUrl is not defined, for example with the app https://cep.test/workbench/applications/hello-world?appVersion=0.0.1

Tested again with hello world and extract app

Hello world app results:
Screenshot 2023-09-15 at 3 28 21 PM

output from hello world:
2023/09/15 15:23:36 info unpack layer: sha256:9baa3e2d5cb22ea740563550f3dfd46582159d921b15e1b2295dfff7a964fec0 INFO: Creating SIF file... Your command line args (appArgs) are: hello world 30 Sleeping for 30 seconds hello world. My name is cyemparala /scratch1/09478/cyemparala/tapis/91c8e65a-9962-4d0f-a24c-43d7639a5033-007

Copy link
Collaborator Author

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

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

made suggested changes

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shayanaijaz shayanaijaz left a comment

Choose a reason for hiding this comment

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

LGTM. But before merging can we also confirm the wording for the description with the design team?

Copy link
Contributor

@sophia-massie sophia-massie left a comment

Choose a reason for hiding this comment

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

Tested locally and LGTM to me!

Copy link
Member

@wesleyboar wesleyboar 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 a remarkable feature. I have a meeting Monday to help figure out generic text for the field description that every app can use.

Updated Text (Monday, 2023-09-25)

Given my new understanding of how generic this text is:

Target Path for __________
The name of the __________ after it is copied to the target system, but before the job is run. Leave this value blank to [just] use the name of the input file.

(Add or remove the word "just" at your discretion.)

Archived Text (Friday, 2023-09-22)

This text is not generic enough, but encapsulates my current understanding.

The name of both the archive file after it is copied to the target system and the directory into which the archive file is unarchived. Leave this value blank to use the name of the input file.

The name of both the archive file after it is copied to the target system and the directory the archive file is unarchived into. Leave this value blank to use the name of the input file.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Not an action item yet. Just recording knowledge for others.

Implemented for some fields, not this one yet (I am told), is a way to hide fields except for advanced users. I hear the design pattern is established, and that we have been using a isHidden metadata field on the tapis definition objects to programmatically hide certain fields.

My source is @rstijerina.

@chandra-tacc
Copy link
Collaborator Author

Not an action item yet. Just recording knowledge for others.

Implemented for some fields, not this one yet (I am told), is a way to hide fields except for advanced users. I hear the design pattern is established, and that we have been using a isHidden metadata field on the tapis definition objects to programmatically hide certain fields.

My source is @rstijerina.

@wesleyboar - isHidden is a UI hint to hide the corresponding field in UI. If something is hidden, it is never shown in UI. There is no advanced vs standard user setting for features.

If you are thinking of a way to collapse/expand advanced features in the job submission, I think that is something we should talk but it is beyond the scope of this PR.

@wesleyboar
Copy link
Member

wesleyboar commented Sep 25, 2023

Thanks @chandra-tacc. Then I misinterpreted what I heard. That's a bummer. Yes, out of scope.

@rstijerina
Copy link
Member

I think you're both right. isHidden is a field we can apply to this as well to make it fall in line with how we hide other "advanced" fields, no?

@chandra-tacc
Copy link
Collaborator Author

I think you're both right. isHidden is a field we can apply to this as well to make it fall in line with how we hide other "advanced" fields, no?

Saw this comment just now.
Based on offline conversation, an advanced features option which sets all or nothing or entire app on whether to show target path or not is the next step.

@chandra-tacc
Copy link
Collaborator Author

Addressed 2 issues:

  • Label adjustment based on design team feedback
  • Show/hide target path based on notes in app definition.

Tested the following apps:

  • Extract
  • Hello World
  • Opensees ( does not show target path)

App Definition change for Extract:

"notes": {
       "label": "Extract Compressed File",
       "hideNodeCountAndCoresPerNode": true,
       "icon": "extract",
       "category": "Utilities",
       "showTargetPath": true
   }

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

5 participants