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

DM-31476: Clean up panda's yaml and provide one default config #50

Merged
merged 4 commits into from Sep 30, 2021

Conversation

hsinfang
Copy link
Contributor

@hsinfang hsinfang commented Sep 29, 2021

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

Copy link
Collaborator

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

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

I've commented on things that jumped out at me. None of those lines I commented on should cause any problems running. Presumably the example_panda_SLAC.yaml and maybe the HSC-PANDA.yaml should also be cleaned up, but that can be done later in a different ticket.
Changes approved

includeConfigs:
- ${CTRL_BPS_DIR}/config/bps_idf.yaml

operator: ${USER}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double check whether this is needed. If operator isn't set in the submit yaml, bps should grab the value of that environment variable right after reading the yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default sounds good. I removed this line.

inCollection: HSC/calib,HSC/raw/all,refcats
dataQuery: "tract = 9615 and patch=30 and detector IN (10..11) and instrument='HSC' and skymap='hsc_rings_v1' and band in ('r')"
output: u/{operator}/pcheck_ebutler
output: "u/{operator}/{payloadName}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the default. I don't object if you want to put it here to make it easy for someone to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave this here as I think we might want to change it in most DP0.2 runs.

fileDistributionEndPoint: "s3://butler-us-central1-panda-dev/hsc/{payload_folder}/{uniqProcName}/"
s3_endpoint_url: "https://storage.googleapis.com"
payload_folder: payload
fileDistributionEndPoint: "s3://butler-us-central1-panda-dev/dc2/{payload_folder}/{uniqProcName}/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this something regular users tend to change? If not it can go into the bps_idf.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most people won't change it but I hesitate if it should match the butler repo. I moved it to bps_idf.yaml.

#PANDA plugin specific settings:
idds_server: "https://aipanda015.cern.ch:443/idds"
placeholderParams: ['qgraphNodeId', 'qgraphId']
defaultPreCmdOpts: "--long-log --log-level=VERBOSE --log-file payload-log.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the line I commented in slack about. defaultPreCmdOpts is in every pipetask and butler command line. This means those commands that are executed during submit will also output that json file. Currently it looks like at least one command is executed while in the cwd and then at least one after switching to the run directory. This is something that probably could be changed in bps, but I'd rather not rush to get it in tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find the side effect of having payload-log.json in cwd inconsistent with the rest of artifacts. But can live with it for now.

@hsinfang
Copy link
Contributor Author

I removed HSC-PANDA.yaml but am not sure about touching example_panda_SLAC.yaml; I'm leaving it to somebody who can test at SLAC's panda.

@hsinfang hsinfang force-pushed the tickets/DM-31476 branch 2 times, most recently from 80c0cd4 to 589024b Compare September 30, 2021 03:56
@hsinfang hsinfang merged commit 7b4afea into master Sep 30, 2021
@hsinfang hsinfang deleted the tickets/DM-31476 branch September 30, 2021 05:44
Copy link
Contributor

@yesw2000 yesw2000 left a comment

Choose a reason for hiding this comment

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

In the file config/bps_idf.yaml, I noticed that the parameter defaultPreCmdOpts is defined, and the parameters runPreCmdOpts and initPreCmdOpts are used but not defined. Did I miss something?

@hsinfang
Copy link
Contributor Author

hsinfang commented Sep 30, 2021

@yesw2000 Don't those follow the defaults @MichelleGower set?

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.

None yet

3 participants