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

Bug/WP-306: If input file target path is empty, do not send it to tapis #871

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

chandra-tacc
Copy link
Collaborator

@chandra-tacc chandra-tacc commented Oct 3, 2023

Overview

A recent change #857 introduced target path for input file. But, this was done only if the app definition requested for it.
When the app definition does not want this feature, the current implementation is overriding target path to "*" always. This is a regression. The production behavior is to not send targetPath back to tapis.

Related

Changes

The behavior in prod is to not send targetPath. This code change will match that to not send target path if it is empty (undefined, null or empty in UI). This will meet all conditions.

Testing

1.Tested OpenSees which was failing before to copy files to right location
Job parameters show no targetPath( this matches prod):

{
    "job": {
        "fileInputs": [
            {
                "name": "TCL Input Directory",
                "sourceUrl": "tapis://cloud.data/corral/tacc/aci/CEP/community/opensees-sp/examples/smallsp"
            }
        ],
        "parameterSet": {
            "appArgs": [
                {
                    "name": "TCL Script",
                    "arg": "Example.tcl"
                }
            ],
            "containerArgs": [],
            "schedulerOptions": [
                {
                    "name": "TACC Allocation",
                    "description": "The TACC allocation associated with this job execution",
                    "include": true,
                    "arg": "-A TACC-ACI"
                }
            ],
            "envVariables": []
        },
        "name": "opensees-sp-v35-0.0.1_2023-10-03T22:56:41",
        "nodeCount": 1,
        "coresPerNode": 4,
        "maxMinutes": 120,
        "archiveSystemId": "frontera",
        "archiveSystemDir": "HOST_EVAL($HOME)/tapis-jobs-archive/${JobCreateDate}/${JobName}-${JobUUID}",
        "archiveOnAppError": true,
        "appId": "opensees-sp-v35",
        "appVersion": "0.0.1",
        "execSystemId": "frontera",
        "execSystemLogicalQueue": "development"
    },
    "licenseType": null,
    "isInteractive": false
}
  1. Tested extract which has target path exposed.
    The job worked

  2. Tested with namd
    It matches the production behavior (compared with frontera). Output location: frontera/home1/09478/cyemparala/tapis-jobs-archive/2023-10-03Z/namd-2.14_2023-10-03T23:44:19-c9521e71-1cbc-420b-9dbb-a249d52a15c5-007

Screenshot 2023-10-03 at 6 55 44 PM

UI

No Change

Notes

Created bug WP-307 to support the default targetPath setup in app definition. We have never used it, and it needs to be tested and any changes should not break apps.

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #871 (e6baf95) into main (67f4e75) will decrease coverage by 0.02%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #871      +/-   ##
==========================================
- Coverage   63.00%   62.98%   -0.02%     
==========================================
  Files         427      427              
  Lines       12191    12196       +5     
  Branches     2504     2506       +2     
==========================================
+ Hits         7681     7682       +1     
- Misses       4303     4306       +3     
- Partials      207      208       +1     
Flag Coverage Δ
javascript 68.67% <25.00%> (-0.04%) ⬇️
unittests 57.29% <ø> (ø)

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

Files Coverage Δ
...nt/src/components/Applications/AppForm/AppForm.jsx 64.50% <0.00%> (-0.29%) ⬇️
...rc/components/Applications/AppForm/AppFormUtils.js 49.29% <33.33%> (-1.46%) ⬇️

@chandra-tacc chandra-tacc marked this pull request as ready for review October 3, 2023 23:33
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

@chandra-tacc chandra-tacc merged commit 7abd8ad into main Oct 5, 2023
4 of 6 checks passed
@chandra-tacc chandra-tacc deleted the bug/WP-306 branch October 5, 2023 16:04
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.

3 participants