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

Pyro client from remote job can use cylc-suite-env #1768

Merged
merged 3 commits into from
Mar 29, 2016

Conversation

matthewrmshin
Copy link
Contributor

A Pyro client invoked by a remote task job can now use host and port information from the cylc-suite-env file. This allows commands such as broadcast to work from a task job running on a remote host without a shared file system with the suite host.

@matthewrmshin matthewrmshin added this to the next-release milestone Mar 18, 2016
@matthewrmshin
Copy link
Contributor Author

@hjoliver @arjclark please review.

self.host = host
self.port = port
except (IOError, ValueError):
pass
Copy link
Member

Choose a reason for hiding this comment

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

The cylc-suite-env file is also read and parsed in lib/cylc/task_message, but with different code. Maybe time for a small module that handles parsing (and writing) the file, just returning a dict of parsed values?

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 did have a look at the logic, and I can have a look again. The write logic is done in 3 lines in cylc.scheduler.Scheduler.configure, (and the dict containing the data is defined in cylc.scheduler.Scheduler.configure_suite_environment in 7 lines). The read/parse logic in cylc.task_message is in 3 lines to override its own environment map, which is slightly different from the purpose here. I'll have a look to see what's the best way to combine everything.

@hjoliver
Copy link
Member

Definitely fixes the problem. One comment above.

A Pyro client invoked by a remote task job can now use host and port
information from the `cylc-suite-env` file.
@matthewrmshin
Copy link
Contributor Author

@hjoliver This is now ready to be looked at again. Note that I have rebased the branch.

suite_owner: the owner user ID of the suite
suite_cylc_version: version of cylc for running the suite

Constansts:
Copy link
Member

Choose a reason for hiding this comment

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

spelling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@hjoliver
Copy link
Member

Review 1 - good, test battery passes here; one trivial spelling error in a comment!

@hjoliver hjoliver assigned matthewrmshin and unassigned hjoliver Mar 22, 2016
@arjclark
Copy link
Contributor

Review 2 - looks ok to me. Test battery behaving as expected.

@arjclark arjclark merged commit 5c77d5f into cylc:master Mar 29, 2016
@matthewrmshin matthewrmshin deleted the pyro-client-from-remote-task-job branch March 29, 2016 14:53
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.

3 participants