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.

Sign up for GitHub

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

Jump to bottom

Sci-embodied redesign #995

Merged
merged 28 commits into from
Aug 28, 2024
Merged

Sci-embodied redesign #995

merged 28 commits into from
Aug 28, 2024

Conversation

narekhovhannisyan
Copy link
Member

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

@narekhovhannisyan
feat(package): update if-core version
cc5c093
@narekhovhannisyan
feat(types): use aggregation from if core
8b7014c
@narekhovhannisyan
feat(util): introduce copy and none methods to aggregation helper
7031c76
@narekhovhannisyan
feat(lib): return none aggregation method
d04de7e
@narekhovhannisyan
revert(config): drop sci embodied message from strings
f742a0b
@narekhovhannisyan
feat(builtins): introduce none and copy aggregation methods
7186365
@narekhovhannisyan
doc(builtins): introduce documentation for sci embodied
115c813
@narekhovhannisyan
feat(builtins): implement upgraded version of sci embodied
3697ae3
@narekhovhannisyan
feat(src): use aggregation methods from if core
06d4045
@narekhovhannisyan
fix(util): fix console log issue in debug logger
5dd9f00
@narekhovhannisyan
feat(types): use aggregation methods from if core
5cd992b
@narekhovhannisyan
test(util): update aggregation usage in aggregation helper
6a31dbf
@narekhovhannisyan
tesst(lib): remove skip from environment, fix typo
c96a03d
@narekhovhannisyan
test(lib): update aggregation usage in aggregation
f557eb7
@narekhovhannisyan
test(builtins): update time sync units
387801c
@narekhovhannisyan
test(builtins): implement units for sci embodied
2365c4c
@narekhovhannisyan
revert(util): remove conole.log from aggregation helper
c43c5eb
@jmcook1186 jmcook1186 changed the title Sci redesign Sci-embodied redesign Aug 27, 2024
@narekhovhannisyan narekhovhannisyan linked an issue Aug 28, 2024 that may be closed by this pull request
Redesign sci-embodied builtin #917
Closed
4 tasks
@narekhovhannisyan narekhovhannisyan self-assigned this Aug 28, 2024
@jmcook1186
Copy link
Contributor

Hi @narekhovhannisyan I made some changes to the behaviour - once I started using it I realised the vcpu/totalcpu ratio feature isn't quite right so I replaced it with a generic usage-ratio factor that can be used for e.g. allocated memory vs totalmemory, or any other arbitrary scalign factor the user wants to provide. Also updated time so it can be user defined or defaulting to duration, and moved it to inputs. It now matches a set of manual calculations I have done offline, and I also manually checked the outcomes from each unit test.

jmcook1186
jmcook1186 approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@jmcook1186 jmcook1186 left a comment

Choose a reason for hiding this comment

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

sgtm after pushing changes

manushak
manushak requested changes Aug 28, 2024
View reviewed changes
manifests/examples/builtins/sci-embodied/scenario-1.yml Outdated Show resolved Hide resolved
manifests/examples/builtins/sci-embodied/scenario-2.yml Outdated Show resolved Hide resolved
src/if-run/builtins/sci-embodied/README.md Outdated Show resolved Hide resolved
src/if-run/builtins/sci-embodied/README.md Outdated Show resolved Hide resolved
src/if-run/builtins/sci-embodied/index.ts

return validate<z.infer<typeof schema>>(schema, input);
const cpuE =
Copy link
Contributor

Choose a reason for hiding this comment

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

move calculation to separate function

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no reason to move it, using safeConfig and safeInput object properties are making whole thing scary

@narekhovhannisyan @manushak
Update src/if-run/builtins/sci-embodied/README.md
314e24a 
Co-authored-by: Manushak Keramyan <[email protected]>
Signed-off-by: Narek Hovhannisyan <[email protected]>
@jmcook1186 jmcook1186 merged commit cb57527 into main Aug 28, 2024
2 checks passed
@jmcook1186 jmcook1186 deleted the sci-redesign branch August 28, 2024 14:28
@jmcook1186 jmcook1186 mentioned this pull request Sep 12, 2024
Open
@narekhovhannisyan narekhovhannisyan mentioned this pull request Sep 23, 2024
Closed
6 tasks
This was referenced Oct 4, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmcook1186 jmcook1186 jmcook1186 approved these changes

@manushak manushak manushak approved these changes

Assignees

@narekhovhannisyan narekhovhannisyan

Labels
None yet
Projects
IF
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redesign sci-embodied builtin
3 participants
@narekhovhannisyan @jmcook1186 @manushak