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

fix process control in direct scheduler #2721

Merged
merged 3 commits into from
Apr 10, 2019

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Apr 6, 2019

fix #2649

The direct scheduler was using ps for checking whether a process is
running. However, ps does not show processes without a controlling
terminal - for this you need to add the -x option:

-x When displaying processes matched by other options, include pro-
cesses which do not have a controlling terminal. This is the
opposite of the -X option. If both -X and -x are specified in
the same command, then ps will use the one which was specified
last.

The direct scheduler was using `ps` for checking whether a command is
running. However, `ps` does not show processes without a controlling
terminal - for this you need to add the -x option:

-x      When displaying processes matched by other options, include pro-
        cesses which do not have a controlling terminal.  This is the
        opposite of the -X option.  If both -X and -x are specified in
        the same command, then ps will use the one which was specified
        last.
@ltalirz ltalirz requested a review from giovannipizzi April 6, 2019 22:25
giovannipizzi
giovannipizzi previously approved these changes Apr 6, 2019
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Winner for the smallest fix length/discussion length ratio

@ltalirz
Copy link
Member Author

ltalirz commented Apr 6, 2019

What I haven't done here is add a test that would catch a regression.
I saw there were some tests in the schedulers/plugins directory but only related to parsing job lists.

Is there also an actual test of a scheduler?

@coveralls
Copy link

coveralls commented Apr 6, 2019

Coverage Status

Coverage remained the same at 70.468% when pulling 3376db0 on ltalirz:issue_2649_direct_scheduler into 00d68f8 on aiidateam:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 70.462% when pulling 5c61790 on ltalirz:issue_2649_direct_scheduler into 677b65d on aiidateam:develop.

@sphuber
Copy link
Contributor

sphuber commented Apr 7, 2019

Why didn't this problem affect me on theossrv5? Is this behavior specific to MacOS or should it also affect Linux?

@giovannipizzi
Copy link
Member

@sphuber I try to answer (with a few more questions) in the issue

@ltalirz
Copy link
Member Author

ltalirz commented Apr 10, 2019

@sphuber @giovannipizzi can this be merged?
this issue breaks my integration tests for two plugins...

@sphuber
Copy link
Contributor

sphuber commented Apr 10, 2019

Could you please add a little bit more information to the comment you added at the ps command. Something that explains why we need to put -x and how it will manifest itself if we don't, i.e. the engine will think that the process is terminated when it is not and will start the retrieval step too early.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 10, 2019

Done!

@sphuber sphuber self-requested a review April 10, 2019 08:36
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Great debugging session 👍

@sphuber sphuber merged commit e3af765 into aiidateam:develop Apr 10, 2019
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.

retrieving files from completed calculation - file missing
4 participants