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

Feature/multi loader #559

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from
Draft

Feature/multi loader #559

wants to merge 29 commits into from

Conversation

nosnelmil
Copy link
Contributor

Summary

A wrapper around loader to run multiple studies at once with additional features like validation, dry-run and log collection

Implementation Notes ⚒️

  • Multi-Loader Entry Point:

    • MultiLoaderRunner houses the main logic of the multi-loader and exposes two execution points: RunDryRun and RunActual
    • multi_loader.go in tools/multiloader/ initializes the MultiLoaderRunner and calls the appropriate execution point

  • Experiment Execution Flow:

    • The multi-loader runner is instantiated with the provided configuration path
    • Execution is split into three phases: pre-execution, execution, and post-execution
      • Pre-Execution: Executes global pre-scripts, sets up directories, and unpacks sub-experiments.
      • Execution: Runs experiments with the generated configurations and appropriate flags and collect loader.go logs.
      • Post-Execution: Executes experiment-specific post-scripts and performs necessary cleanup tasks.
    • If the execution is a dry run, the same flow is followed, but the dryRun flag is set to true when calling loader.go

  • Post Execution:

    • Global post-scripts are executed after the experiments finish
    • Executes Make Clean
    • Temporary folders are removed

External Dependencies 🍀

  • N/A

Breaking API Changes ⚠️

  • N/A

Simply specify none (N/A) if not applicable.

@nosnelmil
add loader dry run
3653fc8 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add loader dry run documentation
3265056 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi-loader config
32bbd7c 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi-loader config reader
0f87b5d 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi loader base
7ff0094 
Signed-off-by: Lenson <[email protected]>

add multi loader base

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add node group struct
a170021 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi loader runner
7273c1d 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
refactor multi loader config
482db6c 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi loader config validators
316e539 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add knative specific config enricher
734b60c 
Signed-off-by: Lenson <[email protected]>

add additional knative platform type

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add base runner entry point
ba7181e 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
refactor multi loader config
56755f8 
Signed-off-by: Lenson <[email protected]>

update multi loader config struct

Signed-off-by: Lenson <[email protected]>
@nosnelmil
update unpack study doc
cd683f5 
Signed-off-by: Lenson <[email protected]>

add unpack study

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add prepare experiment
e53eecc 
Signed-off-by: Lenson <[email protected]>

update experiment config temp path

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add run loader function
68d5000 
Signed-off-by: Lenson <[email protected]>

update log parser

Signed-off-by: Lenson <[email protected]>

update log parser

Signed-off-by: Lenson <[email protected]>

update log parser

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add clean up function
94dca93 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add logs to indicate run status
0b2733c 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
expose entry points for multi loader runner
f4861ed 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi loader runner execution
f6b3547 
Signed-off-by: Lenson <[email protected]>

update default multi loader config path

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add basic config
ab1c7cd 
Signed-off-by: Lenson <[email protected]>

update basic config

Signed-off-by: Lenson <[email protected]>

update basic config

Signed-off-by: Lenson <[email protected]>

add basic configs

Signed-off-by: Lenson <[email protected]>

update base config

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add cpu limit validator
8b44690 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
remove extra knative feature
6b58cc3 
Signed-off-by: Lenson <[email protected]>

remove knative extra features

Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi loader tests
6d83ad8 
Signed-off-by: Lenson <[email protected]>
@nosnelmil
add multi loader documentation
3ff658f 
Signed-off-by: Lenson <[email protected]>
@leokondrashov leokondrashov marked this pull request as draft November 21, 2024 08:26
leokondrashov
leokondrashov reviewed Dec 8, 2024
View reviewed changes
Copy link
Contributor

@leokondrashov leokondrashov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is the first brief look at the code.

Please consider moving the multi-loader exclusive part away from the loader's common package. There is no point in storing a structure for a multi-loader there.

Another thing is gofmt; please set the VS Code to run it on all file saves. There are issues with alignment and whitespace around parentheses.

Please add GitHub workflows for the tests and add e2e tests. You can add separate workflow files for both. In e2e, please check the output hierarchy and the different settings used in the experiments.

Please review each function and ensure that the names make sense to someone who reads them for the first time.

For the failing spellcheck, you will need to add all the failing words (unless you misspelled them) into .github/configs/wordlist.txt.

pkg/common/node_types.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is a good idea to create a separate file in common for node group. It is only used in the multi-loader, so it is already not common.

pkg/common/config_types.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Also quite multi-loader related, you can move it where it is actually use, instead of storing it in common.

pkg/common/validators.go Outdated
}

// Check general multi-loader configuration that applies to all platforms
func CheckMultiLoaderConfig(multiLoaderConfig MultiLoaderConfiguration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very multi-loader specific, please move out of common.

tools/multi_loader/runner/multi_loader_runner.go Outdated
Comment on lines 29 to 36
MultiLoaderConfig common.MultiLoaderConfiguration
NodeGroup common.NodeGroup
DryRunSuccess bool
Verbosity string
IatGeneration bool
Generated bool
DryRun bool
Platform string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run gofmt on all the files. VSCode can do it on each save.

tools/multi_loader/runner/multi_loader_runner.go Outdated
return experiments
}

func (d *MultiLoaderRunner) createNewStudy(study common.LoaderStudy, fileName string) common.LoaderStudy {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very obvious what is going on; why do you supply loader study (by copy, BTW) to the function to create a new study.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leokondrashov leokondrashov leokondrashov left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nosnelmil @leokondrashov