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

chore: Use nullable types in PatchExperiment [DET-6486] #3497

Merged
merged 6 commits into from
Feb 1, 2022
Merged

chore: Use nullable types in PatchExperiment [DET-6486] #3497

merged 6 commits into from
Feb 1, 2022

Conversation

mapmeld
Copy link
Contributor

@mapmeld mapmeld commented Jan 26, 2022

Description

Replace the field masking / multiple queries format of PATCH /api/v1/experiment/:id and instead, following the Models API, accept a PatchExperiment body with nullable types. This allows us to send a partial update { "id": 101, "name": "new_name" } and the other fields will be unchanged.

Checklist

  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@netlify
Copy link

netlify bot commented Jan 26, 2022

✔️ Deploy Preview for determined-ui ready!

🔨 Explore the source changes: abb5727

🔍 Inspect the deploy log: https://app.netlify.com/sites/determined-ui/deploys/61f9782c12ebae00097fe8ea

😎 Browse the preview: https://deploy-preview-3497--determined-ui.netlify.app/

Copy link
Contributor

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

the approach lgtm. I'll check it out more and get back to you in a bit

@@ -90,6 +91,25 @@ message Experiment {
google.protobuf.Int32Value forked_from = 16;
}

// PatchModel is a partial update to an experiment with only id required.
message PatchExperiment {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

@hamidzr hamidzr self-requested a review January 26, 2022 23:25
Copy link
Contributor

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

2022-01-27_13-26-15

already we get some validation which is great. It'd be great if it also prevented users from sending unacceptable patches and not accepting inaccurate values:

could we respond with some sort of error like it used to?
image

@hamidzr hamidzr assigned mapmeld and unassigned hamidzr Jan 28, 2022
@mapmeld
Copy link
Contributor Author

mapmeld commented Jan 31, 2022

Proto (as of v3) drops any unknown fields before passing it to our Go code ( protocolbuffers/protobuf#272 , golang/protobuf#1390 ). The best way that someone can know that their unexpected change didn't happen, at the moment, is to look at the Experiment which we returned and that the field isn't added or updated.

@mapmeld
Copy link
Contributor Author

mapmeld commented Jan 31, 2022

oh I should look into this https://developers.google.com/protocol-buffers/docs/proto3#unknowns

in version 3.5 we reintroduced the preservation of unknown fields to match the proto2 behavior. In versions 3.5 and later, unknown fields are retained during parsing and included in the serialized output

@hamidzr
Copy link
Contributor

hamidzr commented Feb 1, 2022

aah 34

oh I should look into this https://developers.google.com/protocol-buffers/docs/proto3#unknowns

in version 3.5 we reintroduced the preservation of unknown fields to match the proto2 behavior. In versions 3.5 and later, unknown fields are retained during parsing and included in the serialized output

is that how fieldmask retained unknown paths? last image here #3497 (review)
?

looks like all the generated structs in go have an unknownFileds attached to them but that's internal

@mapmeld mapmeld enabled auto-merge (squash) February 1, 2022 18:04
@mapmeld mapmeld merged commit c7baf31 into determined-ai:master Feb 1, 2022
@dannysauer dannysauer modified the milestones: 0.0.102, 0.17.9 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants