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

RemoteInfo should go with ExPyRe #189

Open
gelzinyte opened this issue Nov 22, 2022 · 6 comments
Open

RemoteInfo should go with ExPyRe #189

gelzinyte opened this issue Nov 22, 2022 · 6 comments

Comments

@gelzinyte
Copy link
Contributor

What do you think about moving the RemoteInfo mechanism to ExPyRe?

@bernstei
Copy link
Contributor

I had convinced myself that it shouldn't, but I'm happy to reconsider. Let me look at how it's being done and see if there's an obvious alternative design. Do you have something specific in mind?

@bernstei
Copy link
Contributor

I don't see how to do it fully, at least not with the current design. For example, RemoteInfo.num_inputs_per_queued_job is inherently a quantity that wfl needs, because it's related to how you split up the wfl input iterator into chunks.

However, there are parts of RemoteInfo that are just passed wholesale into ExPyRe functions, like env_vars. We might be able to split RemoteInfo into two parts - the one that tells wfl how to call ExPyRe, and the one that's just passed directly to ExPyRe functions. Let me think about that in more detail.

@bernstei
Copy link
Contributor

More of them are just passed through than I expected, at least by the autoparallelization code:

  • sys_name: passed to ExPyRe.start()
  • resources: passed to ExPyRe.start()
  • pre_cmds: passed to ExPyRe()
  • post_cmds: passed to ExPyRe()
  • env_vars: passed to ExPyRe()
  • input_files: passed to ExPyRe()
  • output_files: passed to ExPyRe()
  • header_extra: passed to ExPyRe.start()
  • exact_fit: passed to ExPyRe.start()
  • partial_node: passed to ExPyRe.start()
  • timeout: passed to ExPyRe.get_results()
  • check_interval: passed to ExPyRe.get_results()
  • check_interval: passed to ExPyRe.get_results()
    There's also the question of whether exact_fit and partial_node belong in resources.

These are not:

  • num_inputs_per_queued_job: used to split iterable
  • job_name: add chunk number, then passed to ExPyRe()
  • skip_failures: used to manage loop over chunk results.

Presumably we can move the top list inside ExPyRe someplace, perhaps moving wfl.autoparallelize.util.get_remote() into ExPyRe as well (although I have to think about whether it's useful without the constraints implicitly placed by the way wfl calls it).

The remaining 3 will remain in wfl, maybe becoming parts of AutoparaInfo (num_inputs_per_remote_job, remote_job_name, skip_remote_job_failures?), or maybe a smaller RemoteInfo type structure that stays in wfl.

Is that what you were thinking?

@bernstei
Copy link
Contributor

bernstei commented Nov 22, 2022

I guess it's worth thinking about whether it'll be more clear if there are 3 separate structures (or one structure containing 3 structures), one for the stuff that's passed to the constructor, one to start, and one to get_results.

@gelzinyte
Copy link
Contributor Author

How about having a RemoteInfo in Expyre and another in wfl that inherits Expyre's RemoteInfo and adds the three wfl arguments? This way syntax from wfl perspective wouldn't even change and ExPyRe could make use of RemoteInfo structure which I think is quite convenient.

That said, I overlooked the bits of RemoteInfo only used by workflow, so this issue came from the thought that RemoteInfo seems more intuitively placed with ExPyRe. But with the suggestion above RemoteInfo would also stay with wfl, so maybe this (mostly cosmetic?) reshuffling isn't worth it after all..

@bernstei
Copy link
Contributor

I agree that it seems intuitive, and it's not unreasonable to say that if you're passing ExPyRe 10 things, they should be in a single structure rather than 10 individual function parameters. However, since they actually get passed to 3 different functions (constructor, start, and get_results), that logic becomes less compelling. I'm OK leaving this issue open for now, or deciding it isn't worth doing.

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

No branches or pull requests

2 participants