Separate config and observation on model execute signature #377
Conversation
demakoff
changed the title
| functional-unit-duration: 1 | ||
| functional-unit-time: minutes | ||
| functional-unit: requests | ||
| carbon: 0.0013944444444444442 |
There was a problem hiding this comment.
carbon was added because of new logic on sci model. Looks like sci example was just a bit outdated. Anyway not related to my changes.
| @@ -30,7 +31,5 @@ graph: | |||
| operational-carbon: 5 | |||
| embodied-carbon: 0.02 | |||
| requests: 100 | |||
| functional-unit-duration: 1 | |||
There was a problem hiding this comment.
Here is exactly an idea of the change, to remove config props pollution on outputs.
| @@ -306,6 +306,123 @@ describe('lib/supercomputer: ', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('passes output from earlier executed model to the later ones as an input', async () => { | |||
There was a problem hiding this comment.
IMO having all the test related entities and logic inside of the test itself increases readability and maintainability.
| @@ -112,10 +111,10 @@ export class ModelsUniverse { | |||
|
|
|||
| const Model = await this.handModel(model, path); | |||
|
|
|||
| const callback = async (graphOptions: GraphOptions) => { | |||
There was a problem hiding this comment.
It was just not relevant to the data comes here...
| /** | ||
| * Flattens config entries. | ||
| */ | ||
| private flattenConfigValues(config: Config) { |
There was a problem hiding this comment.
It seemed a pretty dangerous thing (flattening of config) because would lead to issues once two models will have the same config prop name. With the updated implementation all model configs are absolutely separated from each other.
| const configValues = this.flattenConfigValues(config); | ||
| const nestedConfigValues = this.flattenConfigValues(nestedConfig); | ||
|
|
||
| return inputs.map((input: any) => ({ |
There was a problem hiding this comment.
Exactly here we put everything (observations and all the configs) in one box.
| specificInputs, | ||
| config, | ||
| childrenConfig | ||
| const outputs = await pipeline.reduce( |
There was a problem hiding this comment.
Observatory class was replaced with this reduce function.
I believe now it has less complexity and more readability.
| ); | ||
| await supercomputer.compute(); | ||
|
|
||
| expect(modelsHandbook.getInitializedModel).toHaveBeenNthCalledWith( |
There was a problem hiding this comment.
Additionally, we will check that the model is initialized with a proper config.
|
Hi @demakoff , thanks for looking at this. We will take longer than usual to review this because our first task for @narekhovhannisyan and I is to review @jawache's specification, then we will focus on reviewing the implementation. Then, we are going to be very careful about reviewing this because it will necessarily introduce a lot of breaking changes across several repositories, so we only want to do it once, and we want all the necessary fixes in place first. |
Sure, I understand, no probs at all. Let me know if I can assist in any way... |
|
HI - thanks for the work on this but we decided to go in another direction and refactor the whole codebase into a more functional style. Closing this out. Thanks! |
Types of changes
A description of the changes proposed in the Pull Request