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

Separate config and observation on model execute signature #377

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions examples/ompls/sci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ tags: null
initialize:
models:
- name: sci
kind: builtin
path: ''
kind: plugin
path: '@grnsft/if-models'
model: SciModel
graph:
children:
child:
Expand All @@ -30,7 +31,5 @@ graph:
operational-carbon: 5
embodied-carbon: 0.02
requests: 100
functional-unit-duration: 1
Copy link
Author

Choose a reason for hiding this comment

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

Here is exactly an idea of the change, to remove config props pollution on outputs.

functional-unit-time: minutes
functional-unit: requests
carbon: 0.0013944444444444442
Copy link
Author

Choose a reason for hiding this comment

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

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.

sci: 0.0008366666666666665
115 changes: 0 additions & 115 deletions src/__tests__/unit/lib/observatory.test.ts

This file was deleted.

117 changes: 117 additions & 0 deletions src/__tests__/unit/lib/supercomputer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,123 @@ describe('lib/supercomputer: ', () => {
);
});

it('passes output from earlier executed model to the later ones as an input', async () => {
Copy link
Author

Choose a reason for hiding this comment

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

IMO having all the test related entities and logic inside of the test itself increases readability and maintainability.

const testImpl: any = {
name: 'test-impl',
graph: {
children: {
child: {
pipeline: ['model-one', 'model-two'],
inputs: [{testProp: 5}],
},
},
},
};

const mockModelOneExecutor = jest.fn(() => [
{testProp: 5, 'model-one-output': true},
]);
const mockModelTwoExecutor = jest.fn();

const modelsHandbook = {
getInitializedModel: jest.fn(modelName => {
if (modelName === 'model-one') return {execute: mockModelOneExecutor};
if (modelName === 'model-two') return {execute: mockModelTwoExecutor};
return;
}),
};

const supercomputer = new Supercomputer(
testImpl,
modelsHandbook as unknown as ModelsUniverse
);
await supercomputer.compute();

expect(modelsHandbook.getInitializedModel).toHaveBeenNthCalledWith(
1,
'model-one',
{}
);

expect(modelsHandbook.getInitializedModel).toHaveBeenNthCalledWith(
2,
'model-two',
{}
);

expect(mockModelOneExecutor).toHaveBeenNthCalledWith(
1,
[{testProp: 5}],
{}
);

expect(mockModelTwoExecutor).toHaveBeenNthCalledWith(
1,
[{testProp: 5, 'model-one-output': true}],
{}
);
});

it('runs model execute method with two arguments: inputs and properly merged config', async () => {
const testImpl: any = {
name: 'test-impl',
graph: {
children: {
child: {
pipeline: ['model-one'],
config: {
'model-one': {
'config-prop-one': true,
'config-prop-two': 'test',
},
},
children: {
child: {
config: {
'model-one': {
'config-prop-one': false,
'config-prop-three': 1,
},
},
inputs: [{testProp: 5}],
},
},
},
},
},
};

const mockModelOneExecutor = jest.fn();

const modelsHandbook = {
getInitializedModel: jest.fn(() => ({
execute: mockModelOneExecutor,
})),
};

const supercomputer = new Supercomputer(
testImpl,
modelsHandbook as unknown as ModelsUniverse
);
await supercomputer.compute();

expect(modelsHandbook.getInitializedModel).toHaveBeenNthCalledWith(
Copy link
Author

Choose a reason for hiding this comment

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

Additionally, we will check that the model is initialized with a proper config.

1,
'model-one',
{
'config-prop-one': false,
'config-prop-two': 'test',
'config-prop-three': 1,
}
);

expect(mockModelOneExecutor).toHaveBeenNthCalledWith(1, [{testProp: 5}], {
'config-prop-one': false,
'config-prop-two': 'test',
'config-prop-three': 1,
});
});

it('throws `structure malformed` error if nested children component misses `inputs`.', async () => {
const jsonObj = JSON.stringify(impl);
const implCopy = JSON.parse(jsonObj);
Expand Down
9 changes: 4 additions & 5 deletions src/lib/models-universe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {CONFIG, STRINGS} from '../config';

import {
ClassContainerParams,
GraphOptions,
ImplInitializeModel,
InitalizedModels,
} from '../types/models-universe';
Expand Down Expand Up @@ -100,7 +99,7 @@ export class ModelsUniverse {
* Initializes and registers model.
*/
public async writeDown(modelToInitalize: ImplInitializeModel) {
const {model, path, config, name} = modelToInitalize;
const {model, path, config: initConfig, name} = modelToInitalize;

if (!model) {
throw new ModelCredentialError(MISSING_CLASSNAME);
Expand All @@ -112,10 +111,10 @@ export class ModelsUniverse {

const Model = await this.handModel(model, path);

const callback = async (graphOptions: GraphOptions) => {
Copy link
Author

Choose a reason for hiding this comment

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

It was just not relevant to the data comes here...

const callback = async (nodeConfig: Record<string, any>) => {
const params = {
...config,
...graphOptions,
...initConfig,
...nodeConfig,
};

const initalizedModel = await new Model().configure(params);
Expand Down
34 changes: 0 additions & 34 deletions src/lib/observatory.ts

This file was deleted.

Loading