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

Jump to bottom

Initial job interface to the backend #888

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Initial job interface to the backend #888

wants to merge 8 commits into from
+194 −125

Conversation

javiermtorres
Copy link
Contributor

What's changing

This PR prepares a job interface between the backend and the jobs proper. The idea is to end up moving the subclasses to each of the job definitions, and possibly move some parameters across the job-backend boundary .

Refs #413

How to test it

CI should run without issues.

Additional notes for reviewers

N/A

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

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from 815fc6e to f7f9baa Compare February 17, 2025 16:27
ividal
ividal reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@ividal ividal left a comment

Choose a reason for hiding this comment

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

Draft mode, so just a few initial comments - pending some discussion about what will follow :)

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from f7f9baa to 381ebd3 Compare February 18, 2025 13:42
@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from 381ebd3 to 3fceedb Compare February 18, 2025 19:18
@javiermtorres javiermtorres marked this pull request as ready for review February 18, 2025 19:51
@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from 3fceedb to 176b953 Compare February 19, 2025 09:00
@ividal ividal linked an issue Feb 19, 2025 that may be closed by this pull request
Add a new job type to clarify the job interface #413
Open
@njbrake
Copy link
Contributor

njbrake commented Feb 19, 2025

Can this go in after #911? That PR is about to get merged, and should handle a few of the changes you are working on here.

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from 18c8d3f to e68373e Compare February 20, 2025 10:26
njbrake
njbrake reviewed Feb 20, 2025
View reviewed changes
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 think this PR is still in progress, but adding my review so that I don't have it sitting as "pending" 😆 . Re-request my review when you're ready for me to take another look!

@javiermtorres
Copy link
Contributor Author

I think this PR is still in progress

No, it should be working now. Next PR #939 works on this one, but this one right now is ready for review and was working until recent changes in main.

peteski22
peteski22 reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@peteski22 peteski22 left a comment

Choose a reason for hiding this comment

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

Apologies for the comments, it was my attempt to be helpful, I hope it doesn't seem obstructive. ❤️

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from e68373e to d4f5764 Compare February 20, 2025 14:44
@javiermtorres
Copy link
Contributor Author

@njbrake this PR should be aligned with main now, and include changes for some of @peteski22 comments 👍

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from afc21ae to 6031135 Compare February 20, 2025 15:26
ividal
ividal reviewed Feb 20, 2025
View reviewed changes
lumigator/backend/backend/services/jobs.py Outdated
@@ -57,25 +59,138 @@
JobSpecificRestrictedConfig = type[JobEvalConfig | JobInferenceConfig]


def _set_model_type(request: JobCreate) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ follow-up in new issue alert ⚠️

This is yet one more place where we should be ready to accept any. There are an increasing number of providers of models via APIs. I'm not sure yet how flexible (anything with an _API_KEY? Bit much) and we need to check how it would work with other frequent providers (e.g. Bedrock, which integrating LiteLLM opened up to Lumigator).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT @njbrake has already included litellm, so this should be ok?????

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code is now removed, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. AFAICT LiteLLM should solve our issues here, but let me check with @ividal on Monday.

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-413-job-type-interface branch from 6031135 to 2650889 Compare February 20, 2025 15:47
njbrake
njbrake requested changes Feb 20, 2025
View reviewed changes
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.

Overall, love the changes! I think this further cleans up the Job configuration interface. However, I have some concerns with the changes to the mlflow.py file changes that I think need to be answered before I can approve. After that I'm good to approve!

njbrake
njbrake approved these changes Feb 20, 2025
View reviewed changes
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.

Looks like a few minor fixes remain but I'll give the pre-approve so you're not blocked :)

@javiermtorres javiermtorres mentioned this pull request Feb 21, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteski22 peteski22 peteski22 left review comments

@ividal ividal ividal left review comments

@njbrake njbrake njbrake approved these changes

Assignees
No one assigned
Labels
backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a new job type to clarify the job interface
4 participants
@javiermtorres @njbrake @peteski22 @ividal