Add repeatable test+benchmark workflow, CI artifact upload, and scheduler benchmarks#16
Conversation
Summary of ChangesHello Igor Holt (@igor-holt), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's development and operational capabilities by establishing a robust and repeatable testing and benchmarking infrastructure. It introduces a standardized script for running both unit tests and performance benchmarks, integrates this process into the GitHub Actions CI pipeline, and ensures that all generated reports are archived as build artifacts. Furthermore, it adds specific performance benchmarks for the GPU scheduler, allowing for continuous monitoring and optimization of its core functionalities. This foundational work improves code quality, facilitates performance analysis, and streamlines the development workflow for the AIOS Layer. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant amount of new functionality, including CI/CD workflows, benchmarking, and the core logic for the AIOS agent. The changes are well-structured. My review focuses on improving correctness, robustness, and performance. Key suggestions include fixing issues in the Makefile and benchmark tests, improving error handling in the CLI and GPU discovery, and addressing code duplication and best practices in the agent's main logic. Overall, this is a solid foundation.
|
|
||
| bench: | ||
| mkdir -p reports | ||
| go test ./agent/... -run ^$ -bench . -benchmem -count=2 | tee reports/bench.log |
There was a problem hiding this comment.
The bench target pipes the output of go test to tee. In a Makefile, the exit code of a pipeline is determined by the last command, which is tee. This means that if go test fails, the Makefile will not report an error. To ensure that the failure is caught, you should use set -o pipefail.
set -o pipefail; go test ./agent/... -run ^$ -bench . -benchmem -count=2 | tee reports/bench.log
| gpus := []GPU{{Index: 0, Name: "gpu0", MemoryTotal: 1000}} | ||
| for i := 0; i < b.N; i++ { | ||
| sched := New(gpus) | ||
| _, _ = sched.CreateLease("lease", "user", time.Nanosecond) | ||
| sched.ReapExpired() | ||
| } |
There was a problem hiding this comment.
This benchmark includes the scheduler initialization (New) and lease creation inside the loop, so it measures the combined performance of New, CreateLease, and ReapExpired. To isolate the performance of ReapExpired, you should set up the scheduler with an expired lease outside the loop and use b.ResetTimer() before starting the measurement.
gpus := []GPU{{Index: 0, Name: "gpu0", MemoryTotal: 1000}}
sched := New(gpus)
b.ResetTimer()
for i := 0; i < b.N; i++ {
b.StopTimer()
// Setup: create a lease that is already expired.
sched.leases["lease"] = Lease{ID: "lease", ExpiresAt: time.Now().Add(-time.Second)}
b.StartTimer()
sched.ReapExpired()
}| func requestLease(agent, user string, duration int, launch bool) { | ||
| payload := leaseRequest{User: user, DurationSeconds: duration, LaunchRuntime: launch} | ||
| body, _ := json.Marshal(payload) | ||
| resp, err := http.Post(agent+"/v1/leases", "application/json", bytes.NewReader(body)) | ||
| if err != nil { | ||
| fmt.Printf("error: %v\n", err) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
| data, _ := io.ReadAll(resp.Body) | ||
| fmt.Println(string(data)) | ||
| } |
There was a problem hiding this comment.
Errors from json.Marshal, http.Post, and io.ReadAll are either ignored or not handled properly. This can lead to silent failures. For a CLI tool, it's crucial to check all errors, print a descriptive message to stderr, and exit with a non-zero status code to signal failure to the user or calling scripts.
func requestLease(agent, user string, duration int, launch bool) {
payload := leaseRequest{User: user, DurationSeconds: duration, LaunchRuntime: launch}
body, err := json.Marshal(payload)
if err != nil {
fmt.Fprintf(os.Stderr, "error creating request: %v\n", err)
os.Exit(1)
}
resp, err := http.Post(agent+"/v1/leases", "application/json", bytes.NewReader(body))
if err != nil {
fmt.Fprintf(os.Stderr, "error making request: %v\n", err)
os.Exit(1)
}
defer resp.Body.Close()
data, err := io.ReadAll(resp.Body)
if err != nil {
fmt.Fprintf(os.Stderr, "error reading response: %v\n", err)
os.Exit(1)
}
fmt.Println(string(data))
}| func sendInference(endpoint, model, prompt string) { | ||
| payload := map[string]any{ | ||
| "model": model, | ||
| "messages": []map[string]string{ | ||
| {"role": "user", "content": prompt}, | ||
| }, | ||
| "max_tokens": 64, | ||
| "temperature": 0.2, | ||
| } | ||
| body, _ := json.Marshal(payload) | ||
| client := http.Client{Timeout: time.Second * 60} | ||
| resp, err := client.Post(endpoint, "application/json", bytes.NewReader(body)) | ||
| if err != nil { | ||
| fmt.Printf("error: %v\n", err) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
| data, _ := io.ReadAll(resp.Body) | ||
| fmt.Println(string(data)) | ||
| } |
There was a problem hiding this comment.
Similar to requestLease, errors from json.Marshal, client.Post, and io.ReadAll are not properly handled. This can cause the tool to fail silently or produce no output on error. All potential errors should be checked, with messages printed to stderr and a non-zero exit code on failure.
func sendInference(endpoint, model, prompt string) {
payload := map[string]any{
"model": model,
"messages": []map[string]string{
{"role": "user", "content": prompt},
},
"max_tokens": 64,
"temperature": 0.2,
}
body, err := json.Marshal(payload)
if err != nil {
fmt.Fprintf(os.Stderr, "error creating request: %v\n", err)
os.Exit(1)
}
client := http.Client{Timeout: time.Second * 60}
resp, err := client.Post(endpoint, "application/json", bytes.NewReader(body))
if err != nil {
fmt.Fprintf(os.Stderr, "error making request: %v\n", err)
os.Exit(1)
}
defer resp.Body.Close()
data, err := io.ReadAll(resp.Body)
if err != nil {
fmt.Fprintf(os.Stderr, "error reading response: %v\n", err)
os.Exit(1)
}
fmt.Println(string(data))
}| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: "1.22" |
There was a problem hiding this comment.
To improve the performance of the CI workflow, you can enable caching for Go modules. The actions/setup-go action provides a simple way to do this, which will speed up subsequent runs by caching downloaded dependencies.
- uses: actions/setup-go@v5
with:
go-version: "1.22"
cache: true
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Igor Holt <iholt@mymail.aacc.edu>
|
Igor Holt (@igor-holt) I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you. |
Motivation
Description
./scripts/run_checks.shto run tests (go test -count=2) and Go benchmarks (-bench . -benchmem -count=2) and write output toreports/.agent/internal/scheduler/scheduler_bench_test.goand updateMakefileto include abenchtarget that writesreports/bench.log../scripts/run_checks.shand uploadaios-layer/reportsas an artifact via the workflow in.github/workflows/ci.yml.README.mdand.gitignoreto document the new workflow and ignorereports/; addgo.mod/go.sumentries pulled in bygo mod tidyso tests run cleanly.Testing
go mod tidyto populatego.sumand satisfy dependencies, which completed successfully../scripts/run_checks.shlocally, which rango test -count=2and the benchmarks; unit tests (scheduler) passed and benchmarks completed, with logs written toreports/test.logandreports/bench.log.BenchmarkCreateLease~666–676 ns/op,824 B/op,5 allocs/op;BenchmarkReapExpired~837–851 ns/op,808 B/op,4 allocs/op.Codex Task