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

TkAl All in One tool: don't import crabWrapper #26999

Merged
merged 1 commit into from Jun 4, 2019

Conversation

hroskes
Copy link
Contributor

@hroskes hroskes commented May 29, 2019

PR description:

Basically: don't mess with the environment. This caused 10+ messages like

sh: module: line 1: syntax error: unexpected end of file
sh: error importing function definition for `BASH_FUNC_module'

every time we ran the code.

On slc7 this was a more serious issue, because these lines were added to the output of the call to dasgoclient and causing it not to parse because it wasn't a valid json string.

PR validation:

Certain segments of validateAlignments won't work anymore because they're missing this import, but we don't use the configuration options that lead to those sections. Gregor Mittag tried to implement crab a long time ago but it never worked properly. I am not deleting crabWrapper and associated functionality from validateAlignments.py yet in case this conflicts with the condor migration, but if it doesn't, as far as I'm concerned someone can go ahead and delete it.

@adewit @connorpa @mmusich

if this PR is a backport please specify the original PR:

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26999/10055

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @hroskes (Heshy Roskes) for master.

It involves the following packages:

Alignment/OfflineValidation

@christopheralanwest, @tocheng, @cmsbuild, @franzoni, @tlampen, @pohsun can you please review it and eventually sign? Thanks.
@mschrode, @mmusich, @adewit, @tocheng, @tlampen this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@hroskes hroskes changed the title don't import crabWrapper TkAl All in One tool: don't import crabWrapper May 29, 2019
@christopheralanwest
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 30, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/599/console Started: 2019/05/30 02:33

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f20b5a/599/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3210728
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3210392
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@christopheralanwest
Copy link
Contributor

@adewit @connorpa @mmusich AlCa cannot review this PR in any sensible way. Whether or not it should be merged depends more on whether the subsystem experts want the change. AlCa will sign off if you confirm that the change is indeed desired.

@adewit
Copy link
Contributor

adewit commented May 30, 2019

I'm assuming that @hroskes verified that all the parts of validateAlignments that we actually use still work without this, so fine for me from that point of view. Just to be absolutely sure I'd like @henriettepetersen and @DaveBrun94 to confirm that this import is not used in any way in the HTCondor setup they're working on (I don't think it is, but better safe than sorry ;-) )

@hroskes
Copy link
Contributor Author

hroskes commented May 30, 2019

Yes, the only places where this import is used are sections of the code that only happens if you put jobmode = crab or if add --status on the command line, which is also only supposed to be used to check crab jobs. I didn't want to create conflicts, but Henriette and David can go ahead and delete those lines.

@christopheralanwest
Copy link
Contributor

@henriettepetersen @DaveBrun94 There is a request from @adewit that you confirm this import is not used in any way in the HTCondor setup you are working on.

@DaveBrun94
Copy link
Contributor

@adewit We can confirm that this import is not used in the HTCondor setup

@christopheralanwest
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 4, 2019

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

fabiocos commented Jun 4, 2019

+1

@cmsbuild cmsbuild merged commit d9313af into cms-sw:master Jun 4, 2019
@hroskes hroskes deleted the fix-das-client branch June 4, 2019 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants