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

write trace to file as it comes #635

Merged
merged 2 commits into from
Aug 21, 2024
Merged

write trace to file as it comes #635

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
556f7c5
await chunks
pelikhan Aug 20, 2024
92ccef6
pr feedbakc
pelikhan Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions packages/cli/src/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { capitalize } from "inflection"
import { resolve, join, relative } from "node:path"
import { resolve, join, relative, dirname } from "node:path"
import { isQuiet } from "./log"
import { emptyDir, ensureDir } from "fs-extra"
import { convertDiagnosticsToSARIF } from "./sarif"
Expand All @@ -24,6 +24,7 @@
CLI_RUN_FILES_FOLDER,
ANNOTATION_ERROR_CODE,
GENAI_ANY_REGEX,
TRACE_CHUNK,
} from "../../core/src/constants"
import { isCancelError, errorMessage } from "../../core/src/error"
import { Fragment, GenerationResult } from "../../core/src/generation"
Expand All @@ -36,7 +37,11 @@
JSONSchemaStringifyToTypeScript,
JSONSchemaStringify,
} from "../../core/src/schema"
import { TraceOptions, MarkdownTrace } from "../../core/src/trace"
import {
TraceOptions,
MarkdownTrace,
TraceChunkEvent,
} from "../../core/src/trace"
import {
normalizeFloat,
normalizeInt,
Expand All @@ -51,8 +56,8 @@
azureDevOpsParseEnv,
azureDevOpsUpdatePullRequestDescription,
} from "../../core/src/azuredevops"
import { estimateTokens } from "../../core/src/tokens"
import { resolveTokenEncoder } from "../../core/src/encoders"
import { appendFile, writeFile } from "fs/promises"

export async function runScriptWithExitCode(
scriptId: string,
Expand Down Expand Up @@ -113,6 +118,37 @@
return { exitCode, result }
}

if (out) {
if (removeOut) await emptyDir(out)
await ensureDir(out)
}
if (outTrace && trace) {
await ensureDir(dirname(outTrace))
await writeFile(outTrace, "", { encoding: "utf-8" })
trace.addEventListener(
TRACE_CHUNK,
async (ev) => {
const tev = ev as TraceChunkEvent
await appendFile(outTrace, tev.chunk, { encoding: "utf-8" })
},
false
)
}
if (out && trace) {
const ofn = join(out, "res.trace.md")
if (ofn !== outTrace) {
writeFile(ofn, "", { encoding: "utf-8" })
trace.addEventListener(
TRACE_CHUNK,
async (ev) => {
const tev = ev as TraceChunkEvent
await appendFile(ofn, tev.chunk, { encoding: "utf-8" })
},
false
)
}
}

Check failure on line 150 in packages/cli/src/run.ts

View workflow job for this annotation

GitHub Actions / build 

Using async function in event listener can lead to unhandled promise rejections if the promise is rejected. Consider using a try-catch block inside the event listener to handle possible errors. 🚀
pelikhan marked this conversation as resolved.
Show resolved Hide resolved

Copy link

Choose a reason for hiding this comment

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

The setup for trace writing is only done if 'out' and 'trace' are truthy. However, 'outTrace' is used later in the code without checking if it was set up correctly. This could potentially lead to errors if 'outTrace' was not set up because 'out' or 'trace' were falsy. Please ensure that 'outTrace' is always correctly set up. 🧐

generated by pr-review-commit conditional_trace_setup

const toolFiles: string[] = []
const resolvedFiles = new Set<string>()

Expand Down Expand Up @@ -234,8 +270,7 @@
if (result.status !== "success")
logVerbose(result.statusText ?? result.status)

Copy link

Choose a reason for hiding this comment

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

The line of code that writes the trace content to 'outTrace' has been removed. If 'outTrace' is expected to contain the trace content, this could lead to unexpected behavior. Please ensure that the trace content is written to 'outTrace' as expected. 🕵️‍♀️

generated by pr-review-commit removed_trace_write

if (outTrace) await writeText(outTrace, trace.content)
if (outAnnotations && result.annotations?.length) {

Check failure on line 273 in packages/cli/src/run.ts

View workflow job for this annotation

GitHub Actions / build 

The function call to `writeText(outTrace, trace.content)` has been removed but it's not clear if the functionality has been replaced elsewhere. Please ensure that the functionality is still covered. 🧐
pelikhan marked this conversation as resolved.
Show resolved Hide resolved
if (isJSONLFilename(outAnnotations))
await appendJSONL(outAnnotations, result.annotations)
else
Expand All @@ -262,9 +297,7 @@
const promptjson = result.messages?.length
? JSON.stringify(result.messages, null, 2)
: undefined
if (out) {

Check failure on line 300 in packages/cli/src/run.ts

View workflow job for this annotation

GitHub Actions / build 

The `emptyDir(out)` and `ensureDir(out)` function calls have been moved up in the code but it's not clear why this change was made. If the order of operations is important, please add a comment explaining the change. 📚
pelikhan marked this conversation as resolved.
Show resolved Hide resolved
if (removeOut) await emptyDir(out)
await ensureDir(out)
const jsonf = join(out, `res.json`)
const yamlf = join(out, `res.yaml`)

Expand All @@ -273,7 +306,6 @@
const outputf = mkfn(".output.md")
const outputjson = mkfn(".output.json")
const outputyaml = mkfn(".output.yaml")
const tracef = mkfn(".trace.md")
const annotationf = result.annotations?.length
? mkfn(".annotations.csv")
: undefined
Expand All @@ -289,7 +321,6 @@
await writeText(outputyaml, YAMLStringify(result.json))
}
if (result.text) await writeText(outputf, result.text)
if (trace) await writeText(tracef, trace.content)
if (result.schemas) {
for (const [sname, schema] of Object.entries(result.schemas)) {
await writeText(
Expand Down