feat(eval): add datasets CLI commands - #1911
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #1911 +/- ##
============================================
+ Coverage 96.28% 96.37% +0.09%
============================================
Files 245 253 +8
Lines 12105 12471 +366
============================================
+ Hits 11655 12019 +364
- Misses 450 452 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| handle: async (ctx, flags) => { | ||
| if (!flags["id"]) throw new InputValidationError("required option '--id <id>' not specified"); | ||
|
|
||
| // Publishing an unmodified DRAFT creates a version identical to the last |
There was a problem hiding this comment.
current implementation will allow user to publish a version that is unmodified from the last. If silently publishing unmodified version feels wrong here, we can check draftStatus for UNMODIFIED and use that to prompt/warn/reject publishing.
| // renamed into place once the transfer completes. Streams so a large dataset | ||
| // is never held in memory. A failed or aborted transfer removes the temp file and | ||
| // leaves any existing file at `filePath` untouched. | ||
| async function streamToFile( |
There was a problem hiding this comment.
This seems like it should be in the io package.
| * A downloaded dataset could not be written to its destination. | ||
| * Local and user-fixable: a missing directory, a permission denial, etc. | ||
| */ | ||
| export class DatasetWriteError extends AgentCoreCLIError { |
There was a problem hiding this comment.
Can these exceptions be made more general? Maybe NetworkingError and FileWriteError?
jariy17
left a comment
There was a problem hiding this comment.
Pretty Good! Just look at the error handling for the handlers
| // ingesting has no URL to offer yet. Report the status, which is what tells | ||
| // the caller whether to retry. | ||
| if (!dataset.downloadUrl) { | ||
| throw new DatasetDownloadError( |
There was a problem hiding this comment.
This isn't really a user fault, more of service issue. Should we add a retry mechanism here?
There was a problem hiding this comment.
I've classified this as a SERVICE error. I considered adding a retry mechanism, but it was plausible that after a short-polling loop it still wouldn't be active. I lead towards classifying this as a service-side retryable exception, and informing the user to retry once ACTIVE as the right call. Updated code does exactly that
| try { | ||
| const response = await this.fetch(dataset.downloadUrl, { signal }); | ||
| if (!response.ok) { | ||
| throw new DatasetDownloadError( |
There was a problem hiding this comment.
Again this could be a service issue or an Internal issue.
There was a problem hiding this comment.
I think for this case we're good to leave as service. If we hit !response.ok then we successfully made the http call and got a non-2xx
| }); | ||
| } | ||
| body = response.body; | ||
| } catch (error) { |
There was a problem hiding this comment.
Concern about the catch-all wrap here. Blanket-tagging every unknown fetch failure as SERVICE is a guess, and it can mislabel things:
1. A bug in our own request construction is really INTERNAL, not SERVICE.
2. A runtime or polyfill issue is also INTERNAL.
'd only wrap the cases we can positively attribute to the service, like the non-ok HTTP status above. Let anything unrecognized fall through to fromError so it categorizes based on what it actually observes.
| // renamed into place once the transfer completes. Streams so a large dataset | ||
| // is never held in memory. A failed or aborted transfer removes the temp file and | ||
| // leaves any existing file at `filePath` untouched. | ||
| async function streamToFile( |
There was a problem hiding this comment.
There should be a library package for this
| const interrupt = () => controller.abort(); | ||
| process.once("SIGINT", interrupt); | ||
| try { | ||
| const response = await core.eval.downloadDataset( |
There was a problem hiding this comment.
We should add filePath to the response here. Right now the output is just metadata, so nothing in it confirms where the file was written. Returning { ...dataset, filePath } gives the customer clear confirmation of the download and its location.
180627e to
1b22111
Compare
1b22111 to
f3fe5d7
Compare
Description
Adds command-line CRUDL plus versioning for AgentCore evaluation datasets:
eval dataset updatewill land in follow up PRDataSourceTypeunion onCreateDatasetallowing users to pass in s3 uri or file path for data set creation.source.tsx- Responsible for translating file path in dataSourceType union to the service API expectedinlineExampleformat.inlineExamplesis a dict, with list of JSONL items inside. For user convenience and ability to pass their JSONL draft file as a whole -- we accept filepath, and parse the file intoinlineExampleslist before call to service--file-pathflag conditional logic on delete:eval datasets getwithout file path flag will fetch metadata and print to command line (getDataset API). When file path flag is included, CLI will pull content from pre-signed downloadUrl provided in metadata, and writes content to provided file location (streamed into temp file and renamed into place)src/errors:DatasetDownloadError(SERVICE, covers a missing or unfetchable presigned URL) andDatasetWriteError(USER, covers local write issues)Related Issue
Closes #
Documentation PR
Type of Change
Testing
Manually tested commands using
bun run ...against personal account, happy path and negative validation.Full suite against latest HEAD after rebasing onto refactor:
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
Stack created with GitHub Stacks CLI • Give Feedback 💬