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

Add parallel capable ps sweep #103

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

avdudchenko
Copy link
Contributor

This updates the UI to use the new PS interface (this should only be merged after this PR is merged in watertap main (watertap-org/watertap#1268)

This updates current paramter sweep method to use the new PS interface and also enables use of parallel features.

With this PR we need to also add a way for user to specify how many parallel workers to use- currently its fixed to 1 (or should be by default) but user need to be able to specify number of workers from UI - as for some flowsheets they might want to solve in serial mode only.

@lbianchi-lbl lbianchi-lbl added the Priority:Normal Normal Priority Issue or PR label Jan 18, 2024
@lbianchi-lbl
Copy link
Contributor

This PR will make the UI compatible with the current ("new") PS API. No change is needed in the xyz_ui.py modules in watertap-org/watertap since the parameter sweep call is handled entirely by the backend code here in watertap-org/watertap-ui.

@avdudchenko
Copy link
Contributor Author

This PR will make the UI compatible with the current ("new") PS API. No change is needed in the xyz_ui.py modules in watertap-org/watertap since the parameter sweep call is handled entirely by the backend code here in watertap-org/watertap-ui.

Yeap. The user should not see any real difference unless they specify in build options to use more then 1 core during analysis sweep.

@dangunter
Copy link
Collaborator

I have some comments on this. @avdudchenko can I submit a PR to your branch?
Preview:

  • needs more docs (in general)
  • needs tests
  • needs less magic constants
    Started all 3 but need to finish..
    heads up to @MichaelPesce since many of the changes were probably your code not Alex's

@avdudchenko
Copy link
Contributor Author

I have some comments on this. @avdudchenko can I submit a PR to your branch? Preview:

* needs more docs (in general)

* needs tests

* needs less magic constants
  Started all 3 but need to finish..
  heads up to @MichaelPesce since many of the changes were probably your code not Alex's

Yes @dangunter Please go ahead and either push directly to this PR or create a new one.

Not sure what you were referring when it comes to magic numbers but there are some fixed values (0,1s that are used for True/False interpretation - need to double check I did not change that to be True/False in final PS PR).

Anyway.

@dangunter
Copy link
Collaborator

@avdudchenko @MichaelPesce should we revive or close this? I think the # parallel workers was added elsewhere to the UI options. And perhaps the rest of this as well

@MichaelPesce
Copy link
Collaborator

@dangunter @avdudchenko I believe we have the UI set up to provide the necessary input now (parallel workers/subprocesses), and it seems like this could make running parameter sweeps much more efficient. however I was running into issues when trying to get this working on the backend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants