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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Feature/multi loader #559

wants to merge 7 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.

@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 Show resolved Hide resolved
pkg/common/config_types.go Outdated Show resolved Hide resolved
pkg/common/validators.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch from ee6a717 to 6549a3c Compare December 24, 2024 03:49
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch 4 times, most recently from 12d3e5e to b2c1295 Compare January 6, 2025 09:25
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch 2 times, most recently from 45b325a to 038568c Compare January 14, 2025 02:55
leokondrashov
leokondrashov reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@leokondrashov leokondrashov left a comment

Choose a reason for hiding this comment

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

Great job. I've made couple of comments about minor things, please fix

.github/workflows/e2e_loader.yaml Outdated Show resolved Hide resolved
docs/multi_loader.md Outdated Show resolved Hide resolved
tools/multi_loader/base_loader_config.json Outdated Show resolved Hide resolved
tools/multi_loader/common/constants.go Outdated Show resolved Hide resolved
tools/multi_loader/common/utils.go Show resolved Hide resolved
tools/multi_loader/test_data/example_1_test/dirigent.csv Outdated Show resolved Hide resolved
tools/multi_loader/types/node_types.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch 5 times, most recently from 84acc2a to 2adfc32 Compare January 16, 2025 09:32
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch from c3a70e5 to b53d6df Compare January 16, 2025 10:00
leokondrashov
leokondrashov reviewed Jan 20, 2025
View reviewed changes
docs/multi_loader.md Outdated Show resolved Hide resolved
tools/multi_loader/common/validators.go Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner_test.go Outdated Show resolved Hide resolved
tools/multi_loader/runner/multi_loader_runner_test.go Outdated Show resolved Hide resolved
leokondrashov
leokondrashov reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@leokondrashov leokondrashov left a comment

Choose a reason for hiding this comment

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

LGTM

@cvetkovic please take a look. This is a wrapper tool for the loader. It should replace derelict experiment driver that we have lingering and confusing people. We also have several usability features coming up, like parameter sweep (useful for RPS mode), top measurements, and knative logs collection. If you have other enhancements in mind, please let us know.

@leokondrashov leokondrashov marked this pull request as ready for review January 27, 2025 03:42
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch from 72786d7 to c1156ea Compare January 27, 2025 08:52
cvetkovic
cvetkovic reviewed Jan 28, 2025
View reviewed changes
pkg/common/validators.go Outdated Show resolved Hide resolved
@cvetkovic
Copy link
Contributor

LGTM!

@leokondrashov
Copy link
Contributor

Good. Then, please squash commits (at least to less than 10 if you want to separate some of the contributions), fix the comment by Lazar and we are ready to merge.

@nosnelmil
add multi-loader
3c7e6b5 
Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi-loader config reader

Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi loader base

Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi loader base

Signed-off-by: Lenson <nosnelmil@gmail.com>

add node group struct

Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi loader runner

Signed-off-by: Lenson <nosnelmil@gmail.com>

refactor multi loader config

Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi loader config validators

Signed-off-by: Lenson <nosnelmil@gmail.com>

add knative specific config enricher

Signed-off-by: Lenson <nosnelmil@gmail.com>

add additional knative platform type

Signed-off-by: Lenson <nosnelmil@gmail.com>

add base runner entry point

Signed-off-by: Lenson <nosnelmil@gmail.com>

refactor multi loader config

Signed-off-by: Lenson <nosnelmil@gmail.com>

update multi loader config struct

Signed-off-by: Lenson <nosnelmil@gmail.com>

update unpack study doc

Signed-off-by: Lenson <nosnelmil@gmail.com>

add unpack study

Signed-off-by: Lenson <nosnelmil@gmail.com>

add prepare experiment

Signed-off-by: Lenson <nosnelmil@gmail.com>

update experiment config temp path

Signed-off-by: Lenson <nosnelmil@gmail.com>

add run loader function

Signed-off-by: Lenson <nosnelmil@gmail.com>

update log parser

Signed-off-by: Lenson <nosnelmil@gmail.com>

update log parser

Signed-off-by: Lenson <nosnelmil@gmail.com>

update log parser

Signed-off-by: Lenson <nosnelmil@gmail.com>

add clean up function

Signed-off-by: Lenson <nosnelmil@gmail.com>

add logs to indicate run status

Signed-off-by: Lenson <nosnelmil@gmail.com>

expose entry points for multi loader runner

Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi loader runner execution

Signed-off-by: Lenson <nosnelmil@gmail.com>

update default multi loader config path

Signed-off-by: Lenson <nosnelmil@gmail.com>

add cpu limit validator

Signed-off-by: Lenson <nosnelmil@gmail.com>

remove extra knative feature

Signed-off-by: Lenson <nosnelmil@gmail.com>

remove knative extra features

Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi loader tests

Signed-off-by: Lenson <nosnelmil@gmail.com>

add basic config

Signed-off-by: Lenson <nosnelmil@gmail.com>

update basic config

Signed-off-by: Lenson <nosnelmil@gmail.com>

update basic config

Signed-off-by: Lenson <nosnelmil@gmail.com>

add basic configs

Signed-off-by: Lenson <nosnelmil@gmail.com>

update base config

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil
add multi loader documentation
ca5e2ad 
Signed-off-by: Lenson <nosnelmil@gmail.com>

update docs

Signed-off-by: Lenson <nosnelmil@gmail.com>

fix docs

Signed-off-by: Lenson <nosnelmil@gmail.com>

update documentation

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil
refactor multi loader
fc3ad98 
Signed-off-by: Lenson <nosnelmil@gmail.com>

add multi-loader tests

Signed-off-by: Lenson <nosnelmil@gmail.com>

update test

Signed-off-by: Lenson <nosnelmil@gmail.com>

refactor multi-loader tests

Signed-off-by: Lenson <nosnelmil@gmail.com>

add loader experiment

Signed-off-by: Lenson <nosnelmil@gmail.com>

update logs

Signed-off-by: Lenson <nosnelmil@gmail.com>

update log verbosity

Signed-off-by: Lenson <nosnelmil@gmail.com>

update logs

Signed-off-by: Lenson <nosnelmil@gmail.com>

update logs

Signed-off-by: Lenson <nosnelmil@gmail.com>

rename multiloader driver to runner

Signed-off-by: Lenson <nosnelmil@gmail.com>

refactor common files to multiloader folder

Signed-off-by: Lenson <nosnelmil@gmail.com>

refactor multiloader functions

Signed-off-by: Lenson <nosnelmil@gmail.com>

rename createNewStudy function name

Signed-off-by: Lenson <nosnelmil@gmail.com>

fix formatting

Signed-off-by: Lenson <nosnelmil@gmail.com>

remove extra features

Signed-off-by: Lenson <nosnelmil@gmail.com>

remove extra features

Signed-off-by: Lenson <nosnelmil@gmail.com>

add validation for platform

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil
add failfast flag
69c3c3a 
Signed-off-by: Lenson <nosnelmil@gmail.com>

update failfast flag description

Signed-off-by: Lenson <nosnelmil@gmail.com>

update comments

Signed-off-by: Lenson <nosnelmil@gmail.com>

update wordlist with multiloader specific words

Signed-off-by: Lenson <nosnelmil@gmail.com>

simplify run experiment logic

Signed-off-by: Lenson <nosnelmil@gmail.com>

refactor partial experiment naming

Signed-off-by: Lenson <nosnelmil@gmail.com>

fix wrong indexing

Signed-off-by: Lenson <nosnelmil@gmail.com>

add progress in logging

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil
update tests
91042aa 
Signed-off-by: Lenson <nosnelmil@gmail.com>

update multi loader e2e tests

Signed-off-by: Lenson <nosnelmil@gmail.com>

revert setup.cfg

Signed-off-by: Lenson <nosnelmil@gmail.com>

chmod script

Signed-off-by: Lenson <nosnelmil@gmail.com>

update unit tests

Signed-off-by: Lenson <nosnelmil@gmail.com>

fix e2e test

Signed-off-by: Lenson <nosnelmil@gmail.com>

update tests

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil
minor multi loader fix
bfd17be 
Signed-off-by: Lenson <nosnelmil@gmail.com>

fix incorrect retry logging

Signed-off-by: Lenson <nosnelmil@gmail.com>

remove iat and generated cli args

Signed-off-by: Lenson <nosnelmil@gmail.com>

remove make clean from clean up

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil
update multi loader docs
9bac3c4 
Signed-off-by: Lenson <nosnelmil@gmail.com>

update multi-loader docs

Signed-off-by: Lenson <nosnelmil@gmail.com>
@nosnelmil nosnelmil force-pushed the feature/multi-loader branch from 2afc99c to 9bac3c4 Compare January 31, 2025 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cvetkovic cvetkovic cvetkovic left review comments

@leokondrashov leokondrashov leokondrashov approved these changes

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

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