Initial ReqBench init function #402
Conversation
Reqbench implementation
Add reqbench-init and reqbench commands for ReqBench benchmark
tylerharter
left a comment
There was a problem hiding this comment.
Left some feedback on "init". Lets figure that out, and then later I will give feedback on the actual execution.
| } | ||
| func run_reqbench_init(ctx *cli.Context) error { | ||
| olPath, err := common.GetOlPath(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
fix up spacing, use "go fmt"
| csvFilename := "requirements.csv" | ||
| csvFile, err := os.Create(csvFilename) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
in general, try to wrap errors to give more context, something like this:
fmt.Errorf("reqbench init failed to create file: %w", err)
| return err | ||
| } | ||
| // pull requirements.csv | ||
| csvUrl := "https://raw.githubusercontent.com/open-lambda/ReqBench/refs/heads/main/files/requirements.csv" |
There was a problem hiding this comment.
Add TODO comment to make this configurable.
| return err | ||
| } | ||
|
|
||
| csvFile.Seek(0, 0) // for each line |
There was a problem hiding this comment.
Don't use the same open file. Move download stuff to a helper function somewhere. Then just open it normally, without a seek.
| log.Fatal(err) | ||
| } | ||
| for i := 0; ; i++ { | ||
| repo, err := reader.Read() |
There was a problem hiding this comment.
isn't "repo" actually a whole row? confusing name for it
| } | ||
| // get requirements.txt | ||
| // http.Get https://raw.githubusercontent.com/repo[1]/repo[2]/repo[3] | ||
| txtUrl := "https://raw.githubusercontent.com/" + repo[1] + "/" + repo[2] + "/" + repo[3] |
There was a problem hiding this comment.
don't hardcode col positions. Actually use the header (skipped earlier) to figure out col indexes.
| // get requirements.txt | ||
| // http.Get https://raw.githubusercontent.com/repo[1]/repo[2]/repo[3] | ||
| txtUrl := "https://raw.githubusercontent.com/" + repo[1] + "/" + repo[2] + "/" + repo[3] | ||
| txtResponse, err := http.Get(txtUrl) |
There was a problem hiding this comment.
Do we need to download something else? I think we have the "compiled" field with the pip compiled requirements.txt in the dataset already, right?
| txtUrl := "https://raw.githubusercontent.com/" + repo[1] + "/" + repo[2] + "/" + repo[3] | ||
| txtResponse, err := http.Get(txtUrl) | ||
| if err != nil { | ||
| fmt.Printf("Failed to download requirements for lambda %d: %v\n", i, err) |
There was a problem hiding this comment.
If something like this fails, better to fail the benchmark than having different people getting different results because they missed a warning.
| registryPath := strings.TrimPrefix(common.Conf.Registry, "file://") | ||
| lambdaPath := filepath.Join(registryPath, lambdaName) | ||
|
|
||
| if err := os.MkdirAll(lambdaPath, 0755); err != nil { |
There was a problem hiding this comment.
Does this work? Do other benchmarks in this file still work?
I figured we would need to use the new admin install:
Maybe not?
| continue | ||
| } | ||
|
|
||
| functionCode := `def f(event): |
There was a problem hiding this comment.
Is it going to import anything?
Added the function run_reqbench_init, which: