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

LSFManager: optionally stream worker stdout to master #168

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

tanmaykm
Copy link
Member

@tanmaykm tanmaykm commented Aug 6, 2021

This updates the LSFManager to allow stdout and stderr streams of workers to be streamed (optionally) to master. The bpeek invocation when passed the -f flag behaves similar to tail -f. This is the default set for bpeek_flags. The resultant stream is then passed on to the Distributed module after parsing the worker connection string and some initial error handling for the case where job startup is delayed or failed. The Distributed module then takes over the subsequent display of worker's stdout messages on master.

If streaming of worker's stdout messages is not desired, then the -f flag can be omitted from bpeek_flags. Then bpeek behaves similar to cat and is used only to read the initial connection string.

As a positive side effect of this change, it now also avoids the earlier redirect_stderr() call which could have caused race conditions with multiple async invocations of the bpeek method. Few methods are also renamed to have a lsf_ prefix to avoid unnecessary clashes with other parts of the package.

This updates the LSFManager to allow stdout and stderr streams of workers to be streamed (optionally) to master. The `bpeek` invocation when passed the `-f` flag behaves similar to `tail -f`. This is the default set for `bpeek_flags`. The resultant stream is then passed on to the Distributed module after parsing the worker connection string and some initial error handling for the case where job startup is delayed or failed. The Distributed module then takes over the subsequent display of worker's stdout messages on master.

If streaming of worker's stdout messages is not desired, then the `-f` flag can be omitted from `bpeek_flags`. Then `bpeek` behaves similar to `cat` and is used only to read the initial connection string.

As a positive side effect of this change, it now also avoids the earlier `redirect_stderr()` call which could have caused race conditions with multiple async invocations of the `bpeek` method. Few methods are also renamed to have a `lsf_` prefix to avoid unnecessary clashes with other parts of the package.
@tanmaykm tanmaykm requested a review from bjarthur August 6, 2021 01:58
@bjarthur
Copy link
Collaborator

bjarthur commented Aug 6, 2021

looks good. but what is wrong with bpeek(manager... and _launch(manager...? shouldn't be any clashes as dispatch will choose the correct one based on the type of manager, no? i'd suggest sticking with the function names without the lsf_ prefix.

@tanmaykm
Copy link
Member Author

tanmaykm commented Aug 6, 2021

Thanks for revewing! Yes dispatch would take care, but would likely not serve any purpose? Because internal methods are unlikely to have any common purpose across managers. Also prefixing would avoid having another poorly qualified method of a different manager with same name trigger method ambiguities I felt.

Does this seem reasonable?

@tanmaykm
Copy link
Member Author

Unless there are any further major concerns, can this be merged?

@bjarthur
Copy link
Collaborator

bjarthur commented Aug 10, 2021

i don't want to bikeshed, so i'll merge. will be great to have all i/o displayed by master. thanks!

but i will say that adding ::LSFManager to the type signature of bpeek and _launch is all i believe that is needed to mitigate your concerns about clashes with other parts of the package. an unfortunate omission of mine when i wrote this code. sure, the lsf_ prefix makes it less likely they'll be a name clash, but is not a guarantee. doesn't seem very Julian to me either (Base is littered with underscore prefixes and method overloading).

thanks again!

@bjarthur bjarthur merged commit acab8f0 into master Aug 10, 2021
@bjarthur bjarthur deleted the tan/lsf branch August 10, 2021 12:32
@tanmaykm
Copy link
Member Author

Thanks! I was not so hung up on the naming too... agree with your points too. Maybe we need #58 to avoid such worries.
And many thanks for adding LSFManager to this package!

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.

2 participants