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

DirectScheduler: remove the -e option for bash invocation #5264

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 9, 2021

Fixes #5229

The scheduler implementation was submitting the submit script using bash
with the -e option. This flags toggles the behavior that bash will
exit the script as soon as any of the commands in the script return a
non-zero exit code.

This is in contrast with most (if not all) other implementations that
will continue running the script. This is for a good reason, because
often there are critical lines in the rest of the script that need to be
executed for cleanup. For example, with the current state, if the main
executable returns with a non-zero exit code, the script would exit, and
any lines that were added through the append_text of either the
Computer or the Code will be skipped.

Therefore, we remove the -e flag, which technically changes the
behavior, but it is very unlikely that any clients were relying on
particular parts of the submit script to not be executed in case of an
error.

The scheduler implementation was submitting the submit script using bash
with the `-e` option. This flags toggles the behavior that bash will
exit the script as soon as any of the commands in the script return a
non-zero exit code.

This is in contrast with most (if not all) other implementations that
will continue running the script. This is for a good reason, because
often there are critical lines in the rest of the script that need to be
executed for cleanup. For example, with the current state, if the main
executable returns with a non-zero exit code, the script would exit, and
any lines that were added through the `append_text` of either the
`Computer` or the `Code` will be skipped.

Therefore, we remove the `-e` flag, which technically changes the
behavior, but it is very unlikely that any clients were relying on
particular parts of the submit script to not be executed in case of an
error.
@sphuber sphuber requested a review from ltalirz December 9, 2021 10:30
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Cheers!

@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #5264 (2e06afc) into develop (a4a1651) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5264   +/-   ##
========================================
  Coverage    81.42%   81.42%           
========================================
  Files          529      529           
  Lines        37002    37002           
========================================
  Hits         30127    30127           
  Misses        6875     6875           
Flag Coverage Δ
django 76.88% <100.00%> (+0.01%) ⬆️
sqlalchemy 75.87% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
aiida/schedulers/plugins/direct.py 66.28% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4a1651...2e06afc. Read the comment docs.

@sphuber sphuber merged commit 29915bd into aiidateam:develop Dec 9, 2021
@sphuber sphuber deleted the fix/5229/direct-scheduler-set-e branch December 9, 2021 10:52
ltalirz pushed a commit that referenced this pull request Feb 17, 2022
The scheduler implementation was submitting the submit script using bash
with the `-e` option. This flags toggles the behavior that bash will
exit the script as soon as any of the commands in the script return a
non-zero exit code.

This is in contrast with most (if not all) other implementations that
will continue running the script. This is for a good reason, because
often there are critical lines in the rest of the script that need to be
executed for cleanup. For example, with the current state, if the main
executable returns with a non-zero exit code, the script would exit, and
any lines that were added through the `append_text` of either the
`Computer` or the `Code` will be skipped.

Therefore, we remove the `-e` flag, which technically changes the
behavior, but it is very unlikely that any clients were relying on
particular parts of the submit script to not be executed in case of an
error.

Cherry-pick: 29915bd
@ltalirz
Copy link
Member

ltalirz commented Feb 17, 2022

@sphuber Just fyi, I cherry-picked this into support/1.6.x since this can have nasty consequences. We just ran into it as well - people were sourcing the .bashrc in the _aiidasubmit.sh script, which did error, leaving the user wondering why nothing is happening.

If we still plan to push out a 1.6.6 release, we should definitely include this.

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.

append_text not executed if executable not working
2 participants