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.

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

fix: crash if requested output field for inference doesn't exist in dataset #624

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

ividal
Copy link
Contributor

@ividal ividal commented Jan 14, 2025

What's changing

If a field is requested for outputs and it doesn't exist in the input dataset (which is to be expected), inference crashes (and, by extension, so does annotation).

This PR:

  • copies every content from the original dataset to the new one that should be stored (so the user can always delete the previous one if they prefer)
  • creates or overwrites the desired column. This changes the previous behaviour of keeping whatever was in the existing column (which meant we did inference for nothing if there were already predictions/annotations there).

Closes #585

How to test it

Steps to test the changes:

  1. Upload the csv listed in the issue - keep the dataset id
  2. Launch via API an inference job with any model, using the dataset id
  3. The inference job should succeed.

Additional notes for reviewers

This changes the previous behaviour of keeping whatever was in the existing column (which meant we did inference for nothing if there were already predictions/annotations there).

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

@ividal ividal linked an issue Jan 14, 2025 that may be closed by this pull request
1 task
@ividal ividal force-pushed the 585-output-field branch 2 times, most recently from aaba0d0 to fdd7ddc Compare January 15, 2025 00:06
@ividal ividal marked this pull request as ready for review January 16, 2025 09:37
@ividal
Copy link
Contributor Author

ividal commented Jan 16, 2025

⚠️ Rebased.

Copy link
Member

@aittalam aittalam left a comment

Choose a reason for hiding this comment

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

TY Irina for this PR!

I left a couple of minor comments but once they are addressed (and code from main is merged again 😅) this LGTM 🙏

@ividal ividal added the bug Something isn't working label Jan 16, 2025
@ividal ividal added this to the MVP milestone Jan 16, 2025
Copy link
Contributor

@javiermtorres javiermtorres left a comment

Choose a reason for hiding this comment

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

LGTM

@ividal ividal merged commit b894b29 into main Jan 17, 2025
14 checks passed
@ividal ividal deleted the 585-output-field branch January 17, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Inference does not work when column ground_truth is missing
4 participants