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

feat: [VRD-1120, VRD-1130, VRD-1132] Add finetune() #4085

Merged
merged 18 commits into from
Sep 21, 2023
Merged

Conversation

liuverta
Copy link
Contributor

@liuverta liuverta commented Sep 19, 2023

Impact and Context

Creates the ER for tracking the fine-tuning job, creates the RMV for storing the fine-tuned model, logs datasets and internal attributes, and POSTs /finetuning-job.

Risks and Area of Effect

  • Is this a breaking change?

There's a very touchy refactor of a handful of Client methods, but I covered them in Testing.

Testing

  • Unit test
  • Deployed to dev env
  • Other (explain)

Ran the new test_finetune integration test, along with coverage of entity CRUD methods on the client to check for regressions on the refactor

% pytest finetune test_entities.py registry/test_model.py
==================================================== test session starts ====================================================
platform darwin -- Python 3.7.10, pytest-7.4.2, pluggy-1.2.0                                                                 
rootdir: /Users/miliu/Documents/modeldb/client/verta/tests                                                                   
configfile: pytest.ini                                                                                                       
plugins: xdist-3.3.1, hypothesis-6.79.4                                                                                      
collected 78 items                                                                                                           
                                                                                                                             
finetune/test_finetune.py .                                                                                           [  1%] 
test_entities.py ...........................ss......s.s                                                               [ 50%]
registry/test_model.py .....F..........s.......s......s......s                                                        [100%]

==================================== 1 failed, 69 passed, 8 skipped in 433.59s (0:07:13) ====================================

The one failure on registry/test_model.py::TestModel::test_find is unrelated.

Reverting

  • Contains Migration - Do Not Revert

Revert this PR.

Comment on lines -228 to -231
request = conn.make_proto_request(
"GET", "/api/v2/uac-proxy/organization"
request = conn.make_proto_request("GET", "/api/v2/uac-proxy/organization")
response = conn.must_proto_response(
request, OrganizationV2_pb2.ListOrganizationsV2.Response
)
response = conn.must_proto_response(request, OrganizationV2_pb2.ListOrganizationsV2.Response)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore; black did this.

Copy link

Choose a reason for hiding this comment

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

I mean this was bad

Comment on lines -157 to +159
raise ValueError("cannot provide both `organization_id` and `organization_name`")
raise ValueError(
"cannot provide both `organization_id` and `organization_name`"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore; black did this.

Copy link

Choose a reason for hiding this comment

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

rude

Comment on lines +527 to +528
@staticmethod
def _get_or_create_project(
Copy link
Contributor Author

@liuverta liuverta Sep 19, 2023

Choose a reason for hiding this comment

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

This whole file's changes are basically factoring out static versions of these get_or_create methods so that they can be called without needing to instantiate a new Client. See the absolute nightmare that is Client.__init__()—this company's oldest extant Python function—for why we want to avoid that.

Ideally these would actually be moved to new classes—which I have vague, vague plans for—but I wanted to keep this diff as readable as possible for now.

Comment on lines -1765 to +1773
def create_external_build(self, location: str, requires_root: Optional[bool] = None, scan_external: Optional[bool] = None, self_contained: Optional[bool] = None) -> Build:
def create_external_build(
self,
location: str,
requires_root: Optional[bool] = None,
scan_external: Optional[bool] = None,
self_contained: Optional[bool] = None,
) -> Build:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore; black did this.

Comment on lines -1787 to +1803
return Build._create_external(self._conn, self.workspace, self.id, location, requires_root, scan_external, self_contained)
return Build._create_external(
self._conn,
self.workspace,
self.id,
location,
requires_root,
scan_external,
self_contained,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore; black did this.

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

Common Code Coverage

Overall Project 15.01% 🍏

There is no coverage information present for the Files changed

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

Backend Code Coverage

Overall Project 61.32% 🍏

There is no coverage information present for the Files changed

Base automatically changed from liu/lora-config to main September 19, 2023 18:54
Comment on lines +655 to +656
if id is None and self._ctx.proj is None:
self.set_project()
Copy link
Contributor Author

@liuverta liuverta Sep 19, 2023

Choose a reason for hiding this comment

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

This is moved here from the old lines 634–635 below, because the static method can't access self.set_project()

@liuverta liuverta changed the title feat: [VRD-1120] Add finetune() feat: [VRD-1120, VRD-1130, VRD-1114] Add finetune() Sep 20, 2023
@liuverta liuverta changed the title feat: [VRD-1120, VRD-1130, VRD-1114] Add finetune() feat: [VRD-1120, VRD-1130, VRD-1132] Add finetune() Sep 20, 2023
:class:`~verta.registry.entities.RegisteredModelVersion`
New fine-tuned model version.

"""
Copy link
Contributor Author

@liuverta liuverta Sep 20, 2023

Choose a reason for hiding this comment

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

Screenshot 2023-09-21 at 10 15 11 AM

@liuverta liuverta marked this pull request as ready for review September 20, 2023 23:07
@liuverta liuverta requested review from conradoverta, a user and jared-verta September 20, 2023 23:08
],
train_dataset: _dataset_version.DatasetVersion,
eval_dataset: Optional[_dataset_version.DatasetVersion] = None,
test_dataset: Optional[_dataset_version.DatasetVersion] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not actually sure whether we want/need test_dataset. I included it because it felt reasonable for calculating end-of-training performance, but eval_dataset might just serve the same purpose.

Copy link

Choose a reason for hiding this comment

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

I say leave it 🤷🏼‍♀️

# TODO: [VRD-1131] check `enable_mdb_versioning`

ctx = _Context(self._conn, self._conf)
ctx.workspace_name = self.workspace
Copy link

Choose a reason for hiding this comment

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

Do you know if we'll also need to handle orgs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shoot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've verified that Grpc-Metadata-organization-id is faithfully sent on every request. It's encapsulated in conn.

@liuverta liuverta merged commit f462615 into main Sep 21, 2023
1 check failed
@liuverta liuverta deleted the liu/finetune branch September 21, 2023 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants