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

lib: simplify supercomputer calculation logic #381

Conversation

demakoff
Copy link

@demakoff demakoff commented Jan 9, 2024

Types of changes

  • Enhancement (project structure, spelling, grammar, formatting)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

A description of the changes proposed in the Pull Request

  • Before these changes, processing of top-level and nested children was mixed in calculateOutputsForChild method, which led to a number of conditions with areChildrenNested.
  • Now top-level children-specific logic is moved to a compute method while nested children processing logic was extracted to computeChildren. It allowed to unify processing of all children inside of calculateOutputsForChild method and got rid of all areChildrenNested conditions.

Signed-off-by: Dima Demakov <d.demakov@gmail.com>
* Otherwise enriches inputs, passes them to Observatory.
* For each model from pipeline Observatory gathers inputs. Then results are stored.
*/
private async calculateOutputsForChild(
childrenObject: Children,
params: any
childName: string
Copy link
Author

Choose a reason for hiding this comment

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

Since we dropped areChildrenNested, the second object param was replaced with a string childName

@@ -25,7 +25,7 @@ export type Children = {
pipeline: string[];
config: Config;
inputs: ModelParams[];
children: Children;
children?: Children;
Copy link
Author

Choose a reason for hiding this comment

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

We don't have children in the leaves of the children tree.

@demakoff
Copy link
Author

demakoff commented Jan 9, 2024

@jmcook1186 @narekhovhannisyan all tests passed, but could you plz double-check that I didn't miss anything around business logic?

@narekhovhannisyan
Copy link
Member

Hi @demakoff, thanks for your contribution to our project ✨ ! Currently based on Impact Framework's business goals, supercomputer implementation is going to change in the short period of time, and changes from your PR will conflict with it. Therefore it can't be merged. Thanks again for your contribution to our project.

cc: @jmcook1186

@demakoff
Copy link
Author

Hi @demakoff, thanks for your contribution to our project ✨ ! Currently based on Impact Framework's business goals, supercomputer implementation is going to change in the short period of time, and changes from your PR will conflict with it. Therefore it can't be merged. Thanks again for your contribution to our project.

cc: @jmcook1186

Sure, np.
Yeah, I see that you already applied my code here:
https://github.com/Green-Software-Foundation/if/pull/390/files

Pleased that it works properly.

Guys, @jmcook1186 @narekhovhannisyan is there something I can help with (which actually may be merged)?

@jmcook1186
Copy link
Contributor

Hi @demakoff

yes, there are tasks you can pick up for sure. We should make better use of labels to signal which tasks are best for community contributions. I'll add the help-wanted label to some tasks today.

There is also a hackathon coming up and we are putting together suggestions for things we want people to build. We're still refining the ideas to make them easier for people to pick up, but you can take a look at them on the discussion board here: https://github.com/Green-Software-Foundation/hack/discussions

@jmcook1186
Copy link
Contributor

closing as fixed in #390

@jmcook1186 jmcook1186 closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants