(PXP-6544) Limited PFB Export from the Files Tab #729

mpingram · 2020-09-02T22:09:58Z

Link to Jira ticket: PXP-6544
Link to design doc: https://docs.google.com/document/d/12FkAYOpDuSdQScEgYBxXsPUdm8GUuUZgD6xU7EQga5A/edit#heading=h.53vab1pwrz1y
Link to relevant Pelican PR: uc-cdis/pelican#33
Link to manual test plan: uc-cdis/gen3-qa#454

Adds a limited ability to export PFBs of data files from the Files tab. The limitation is that users cannot export data files of different data types (e.g. Aligned Reads, Imaging Files, etc.). (More specifically, users cannot export data files that are on different nodes in the graph.) We're accepting this limitation in order to avoid a significant rewrite of the pelican-export job, which assumes all entities to export are on the same node.

Deployed in https://mpingram.planx-pla.net/. Please feel free to run the test plan (uc-cdis/gen3-qa#454)

New Features

Adds limited support for PFB Export (Download PFB / Export to Terra / Export to Seven Bridges) from the Files Tab. Users can export PFBs of data files of a single data type, but cannot export PFBs of files of multiple data types at the same time.

Breaking Changes

Bug Fixes

Improvements

Error messages now appear if a PFB export fails.
When a PFB download succeeds, shows a link to the PFB instead of just the URL of the PFB.

Dependency updates

Deployment changes

Limited File PFB Export requires Pelican >= 0.5.0, Tube >= 0.4.1
Limited File PFB Export is enabled by setting fileExplorerConfig.enableLimitedFilePFBExport: { sourceNodeField: "source_node" } and by adding buttons of buttonType export-files (Export to Terra), export-files-to-pfb (Download PFB), or export-files-to-seven-bridges (Export to Seven Bridges) to fileExplorerConfig.buttons. Example:

  "fileExplorerConfig": {
    ...
    "buttons": [
      ....
      {
        "enabled": true,
        "type": "export-files-to-pfb",
        "title": "Export All to PFB",
        "rightIcon": "external-link",
        "tooltipText": "You have not selected any cases to export. Please use the checkboxes on the left to select specific cases you would like to export."
      },
      {
        "enabled": true,
        "type": "export-files",
        "title": "Export All to Terra",
        "rightIcon": "external-link",
        "tooltipText": "You have not selected any cases to export. Please use the checkboxes on the left to select specific cases you would like to export."
      }
    ],
    "enableLimitedFilePFBExport": {"sourceNodeField": "source_node"},

Limited File PFB Export requires the source_node property to be ETL'd -- source_node should be added to props in the file section of etlMapping.yaml. Example:

  - name: mpingram_file
    doc_type: file
    type: collector
    root: None
    category: data_file
    props:
      - name: object_id
      - name: md5sum
      ...
      - name: source_node

Limited File PFB Export requires a new export-files job block to be added to the Sower config in manifest.json. The $ROOT_NODE environment variable must be set to "file", or the name of the Guppy file index if it isn't "file". For BioDataCatalyst, the EXTRA_NODES environment variable must be set to "". (This is for backwards compatibility: pelican-export includes reference_file by default on all BDCat PFB exports unless $EXTRA_NODES is set to an empty string). Example sower config:

    {
      "name": "pelican-export-files",
      "action": "export-files",
      "container": {
        "name": "job-task",
        "image": "quay.io/cdis/pelican-export:0.5.0",
        "pull_policy": "Always",
        "env": [
          {
            "name": "DICTIONARY_URL",
            "valueFrom": {
              "configMapKeyRef": {
                "name": "manifest-global",
                "key": "dictionary_url"
              }
            }
          },
          {
            "name": "GEN3_HOSTNAME",
            "valueFrom": {
              "configMapKeyRef": {
                "name": "manifest-global",
                "key": "hostname"
              }
            }
          },
          {
            "name": "ROOT_NODE",
            "value": "file"
          },
          {
            "name": "EXTRA_NODES",
            "value": ""
          }
        ],
        "volumeMounts": [
          {
            "name": "pelican-creds-volume",
            "readOnly": true,
            "mountPath": "/pelican-creds.json",
            "subPath": "config.json"
          },
          {
            "name": "peregrine-creds-volume",
            "readOnly": true,
            "mountPath": "/peregrine-creds.json",
            "subPath": "creds.json"
          }
        ],
        "cpu-limit": "1",
        "memory-limit": "4Gi"
      },
      "volumes": [
        {
          "name": "pelican-creds-volume",
          "secret": {
            "secretName": "pelicanservice-g3auto"
          }
        },
        {
          "name": "peregrine-creds-volume",
          "secret": {
            "secretName": "peregrine-creds"
          }
        }
      ],
      "restart_policy": "Never"
    },

- Reason: easier time configuring specific behavior for specific tabs (e.g. Data and Files tab). For example, if the `export` button type is present it triggers `refreshManifestEntryCount` behavior, which is not needed for the files tab.

lgtm-com · 2020-09-02T22:14:19Z

This pull request introduces 2 alerts when merging b7099ad into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

lgtm-com · 2020-09-03T16:46:49Z

This pull request introduces 2 alerts when merging 38ebc57 into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

lgtm-com · 2020-09-03T19:20:53Z

This pull request introduces 3 alerts when merging 376aa8e into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update
1 for Unused variable, import, function or class

lgtm-com · 2020-09-03T19:26:18Z

This pull request introduces 2 alerts when merging c2b7a70 into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

lgtm-com · 2020-09-03T21:06:49Z

This pull request introduces 2 alerts when merging 1200fdc into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

lgtm-com · 2020-09-03T21:16:08Z

This pull request introduces 2 alerts when merging 425e92a into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

lgtm-com · 2020-09-03T21:26:10Z

This pull request introduces 2 alerts when merging 7b8c61c into 38e68f7 - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

mfshao

generally lgtm, only 1 minor comment

mfshao · 2020-09-03T21:49:45Z

src/GuppyDataExplorer/ExplorerButtonGroup/index.jsx

+    if (buttonConfig.type === 'export-files-to-pfb') {
+      // disable the pfb export button if any other pfb export jobs are running
+      const otherPFBJobsRunning = this.state.exportingToTerra
+        || this.state.exportingToSevenBridges;


why we don't check for isPFBRunning() in here (also in export-to-pfb)?

and there are so many trios of this.state.exportingToTerra || this.state.exportingToSevenBridges || this.isPFBRunning(), maybe we should consolidate them 🤔

why we don't check for isPFBRunning() in here (also in export-to-pfb)?

I wasn't sure why and I was scared to change it 😆. But I can't imagine how adding that extra check for isPFBRunning() would break anything, so I'll consolidate the check for this.state.exportingToTerra || this.state.exportingToSevenBridges || this.isPFBRunning() into a single function.

lgtm-com · 2020-09-04T15:48:24Z

This pull request introduces 2 alerts when merging fb8dd05 into 0ce11bb - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

ZakirG · 2020-09-04T16:58:51Z

src/GuppyDataExplorer/ExplorerButtonGroup/index.jsx

@@ -494,9 +622,28 @@ class ExplorerButtonGroup extends React.Component {
        || this.state.exportingToSevenBridges
        || this.isPFBRunning());
    }
+    if (buttonConfig.type === 'export-files') {
+      if (!this.props.buttonConfig.terraExportURL) {
+        console.error('Export to Terra button is present, but there is no `terraExportURL` specified in the portal config. Disabling the export to Terra button.'); // eslint-disable-line no-console


something worth noting: each re-render (like clicking a checkbox) produces this error print 12 times :O

if it's too much work, no worries, but -- the constructor might be a better place to do these error prints

Ya good idea, I just stuck these checks in the constructor!

ZakirG

Looks good. I tested with an unaligned reads PFB, downloaded the PFB, checked the contents, it matches what I expected. Left 2 comments heads up ~

…n once

lgtm-com · 2020-09-04T18:52:47Z

This pull request introduces 2 alerts when merging 49f5dd7 into 0ce11bb - view on LGTM.com

new alerts:

2 for Potentially inconsistent state update

mpingram added 7 commits August 31, 2020 14:35

Export file PFB if all selected files on same node

0f919d6

Clean up code

24d1b62

Add portal configuration

588baac

Add tooltip if file pfb export button is disabled

2bbcf9d

Show error message on failed PFB export

e1b9517

Revert disabling refreshManifestEntryCount

23f1f66

Clean up code

38ebc57

mpingram mentioned this pull request Sep 3, 2020

(PXP-6544) Pelican-export allows client to specify the root node uc-cdis/pelican#33

Merged

mpingram marked this pull request as ready for review September 3, 2020 17:08

mpingram added 2 commits September 3, 2020 14:14

Use job status for error messages instead of FETCH_ERROR

376aa8e

Revert mistaken debugging change

c2b7a70

Fix bug: disable other pfb buttons when file pfb request in-flight

1200fdc

Make export File PFB tooltip text less confusing.

425e92a

Revert mistakenly uploaded files

7b8c61c

mfshao reviewed Sep 3, 2020

View reviewed changes

Merge branch 'master' into feat/limited-pfb-file-export

fb8dd05

ZakirG reviewed Sep 4, 2020

View reviewed changes

ZakirG approved these changes Sep 4, 2020

View reviewed changes

mpingram added 2 commits September 4, 2020 13:36

Refactor: consolidate duplicated code in isButtonEnabled()

c55a4e9

Refactor: move misconfiguration checks to constructor so they only ru…

49f5dd7

…n once

PlanXCyborg mentioned this pull request Aug 20, 2021

Chore/another dummy pr uc-cdis/gitops-qa#1592

Closed

This was referenced Sep 13, 2021

BDCat prod release 1631557981 uc-cdis/cdis-manifest#3484

Merged

BDCAT Staging Release 2021.07 staging.gen3.biodatacatalyst.nhlbi.nih.gov 1631640599 uc-cdis/cdis-manifest#3495

Merged

deploying 2021.09 to QA NCT uc-cdis/gitops-qa#1627

Merged

This was referenced Sep 20, 2021

Deploying 2021.09 to PROD NCT uc-cdis/cdis-manifest#3532

Merged

Gen3 Monthly Release 2021.09 staging.midrc.org 1632441341 uc-cdis/cdis-manifest#3550

Closed

PlanXCyborg mentioned this pull request Sep 28, 2021

chore : Deploy release 2021.09 and audit-service to IBDGC PROD uc-cdis/cdis-manifest#3563

Merged

PlanXCyborg mentioned this pull request Oct 13, 2021

Gen3 Monthly Release 2021.10 staging.midrc.org 1634083502 uc-cdis/cdis-manifest#3641

Closed

PlanXCyborg mentioned this pull request Oct 20, 2021

Feat/midrc staging uc-cdis/cdis-manifest#3668

Merged

PlanXCyborg mentioned this pull request Dec 10, 2021

Gen3 Monthly Release 2021.11 dataguids.org 1638231984 uc-cdis/cdis-manifest#3847

Closed

PlanXCyborg mentioned this pull request Jan 11, 2022

Gen3 Monthly Release 2022.01 dataguids.org 1641863795 uc-cdis/cdis-manifest#3986

Closed

PlanXCyborg mentioned this pull request Jan 20, 2022

Deploying 2021.12 ibdgc.datacommons.io 1642692655 -l gen3-release uc-cdis/cdis-manifest#4058

Merged

This was referenced Jan 25, 2022

Enable export PFB to URL in Files tab #965

Closed

Enable export PFB to URL in Files tab #966

Merged

PlanXCyborg mentioned this pull request Feb 3, 2022

Gen3 Monthly Release 2022.02 dataguids.org 1643908147 uc-cdis/cdis-manifest#4119

Closed

PlanXCyborg mentioned this pull request Mar 2, 2022

Gen3 Monthly Release 2022.03 dataguids.org 1646179627 uc-cdis/cdis-manifest#4228

Merged

PlanXCyborg mentioned this pull request Mar 18, 2022

internal-icgc pcawg 2022.03 2021.06 internal-icgc.bionimbus.org 1647626564 uc-cdis/cdis-manifest#4337

Merged

PlanXCyborg mentioned this pull request Oct 21, 2022

Gen3 Monthly Release 2022.10 dcf-interop.kidsfirstdrc.org 1666320713 uc-cdis/cdis-manifest#5281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PXP-6544) Limited PFB Export from the Files Tab #729

(PXP-6544) Limited PFB Export from the Files Tab #729

mpingram commented Sep 2, 2020 •

edited

Loading

lgtm-com bot commented Sep 2, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

mfshao left a comment

mfshao Sep 3, 2020

mpingram Sep 4, 2020 •

edited

Loading

lgtm-com bot commented Sep 4, 2020

ZakirG Sep 4, 2020

ZakirG Sep 4, 2020 •

edited

Loading

mpingram Sep 4, 2020

ZakirG left a comment

lgtm-com bot commented Sep 4, 2020

(PXP-6544) Limited PFB Export from the Files Tab #729

(PXP-6544) Limited PFB Export from the Files Tab #729

Conversation

mpingram commented Sep 2, 2020 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

lgtm-com bot commented Sep 2, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

lgtm-com bot commented Sep 3, 2020

mfshao left a comment

Choose a reason for hiding this comment

mfshao Sep 3, 2020

Choose a reason for hiding this comment

mpingram Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Sep 4, 2020

ZakirG Sep 4, 2020

Choose a reason for hiding this comment

ZakirG Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

mpingram Sep 4, 2020

Choose a reason for hiding this comment

ZakirG left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 4, 2020

mpingram commented Sep 2, 2020 •

edited

Loading

mpingram Sep 4, 2020 •

edited

Loading

ZakirG Sep 4, 2020 •

edited

Loading