Skip to content

Conversation

@peteski22
Copy link
Contributor

@peteski22 peteski22 commented Mar 20, 2025

What's changing

  • Fixes the schemas: InferenceJobOutput, InferenceMetrics, PredictionResult and JobOutput within jobs/
  • Removes the extra list comprehension in metrics calculation, extracts average calculation to function and adds tests
  • Align S3 paths for jobs, try to re-use functions to create URIs and pass in fully qualified storage paths when submitting jobs
  • Sanitize S3 filename to prevent forward slashes and other awkward characters
  • Inference and Evaluation jobs updated (mostly based on S3 tweaks)

How to test it

Steps to test the changes:

Additional notes for reviewers

Anything you'd like to add to help the reviewer understand the changes you're proposing.

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required

@peteski22 peteski22 requested a review from njbrake March 20, 2025 17:37
@peteski22 peteski22 enabled auto-merge (squash) March 20, 2025 17:38
Copy link
Contributor

@njbrake njbrake left a comment

Choose a reason for hiding this comment

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

I'm interested to understand why the change to setting optional values as 0: it seems like it may quietly ignore errors by setting defaults when missing, instead of explicitly checking for values that are missing.

@peteski22 peteski22 mentioned this pull request Mar 21, 2025
4 tasks
@peteski22 peteski22 requested a review from njbrake March 26, 2025 12:22
Copy link
Contributor

@njbrake njbrake left a comment

Choose a reason for hiding this comment

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

A few scope-creepy type comments but overall looks great. Thank you for the improvements!

@peteski22 peteski22 force-pushed the jobs/inference/run_inference-minor-tweaks branch 2 times, most recently from 7429631 to bdce671 Compare March 27, 2025 09:55
@peteski22 peteski22 disabled auto-merge March 27, 2025 11:08
@github-actions github-actions bot added the schemas Changes to schemas (which may be public facing) label Mar 27, 2025
@peteski22 peteski22 force-pushed the jobs/inference/run_inference-minor-tweaks branch from 3103e65 to e39303e Compare March 27, 2025 19:00
@peteski22 peteski22 changed the title Tweak schema: Update InferenceJobOutput schema type and tweak inference metrics calc Update schemas, align job results S3 paths, refactor inference metrics calc Mar 27, 2025
@peteski22 peteski22 force-pushed the jobs/inference/run_inference-minor-tweaks branch from e39303e to e850c1a Compare March 27, 2025 19:23
@github-actions github-actions bot added the sdk label Mar 27, 2025
…orage paths

* Adjust 'jobs' schemas (JobOutput) to allow missing (and None) metrics
* Sanitize S3 filename to prevent forward slashes and other awkward characters
* Inference and Evaluation jobs updated (mostly based on S3 tweaks)
@peteski22 peteski22 force-pushed the jobs/inference/run_inference-minor-tweaks branch from e850c1a to a2a790f Compare March 27, 2025 19:47
@peteski22 peteski22 merged commit b37394d into main Mar 27, 2025
22 checks passed
@peteski22 peteski22 deleted the jobs/inference/run_inference-minor-tweaks branch March 27, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend schemas Changes to schemas (which may be public facing) sdk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants