Api definition guidelines #71

gabe-l-hart · 2024-05-31T20:55:51Z

Description

This PR introduces a framework for managing OpenAPI definitions for InstructLab APIs and Platform APIs.

russellb · 2024-05-31T22:06:17Z

This looks pretty good to me, thanks!

My main request is that we merge #69 first (should be able to next week) and then we treat this as one of the first artifacts coming out of that working group.

The only change I'd suggest then is to move the doc/api-definition-guidelines.md file under docs/backend/, but that requires #69 to be in first.

There is a bit of additional context I'd like to capture, but I'm OK not blocking this PR on it. For example, this is written with an architecture in mind about an InstrutLab API and supporting platform APIs. I'd like to write a high level backend direction doc that would introduce these concepts. I have existing content for this. If feedback on that doc changes anything substantially, we can always adjust these docs too. It's all a moving target.

russellb · 2024-05-31T22:06:44Z

The hold label is just for waiting on #69.

gabe-l-hart · 2024-06-03T14:34:51Z

That sounds good. I have an internal platform ADR about the decision to make REST APIs as the common service layer that we can adapt for the general backend architecture perspective.

russellb · 2024-06-04T00:58:36Z

#69 merged, so I'll drop the hold

russellb

This looks good to me, though I'd like to get some other reviews on it before we merge it.

One point that may be worth adding is something like:

We make no guarantees about any type of API stability at this time as we are still working on the initial implementations of these APIs. As implementation progresses, we may decide to make significant breaking changes to what you find here.

russellb

I'm happy with this.

I'm still a bit surprised that nobody else has reviewed it though. Maybe we aren't making it visible in enough of the right places?

README.md

leseb

Thanks for getting started on the API definition!

docs/backend/api-definitions-guidelines.md

hickeyma

This is looking good, thanks for pushing this @gabe-l-hart. Some small nits inline and questions around where specifications should reside.

README.md

hickeyma · 2024-06-07T09:23:18Z

api-definitions/README.md

@@ -0,0 +1,23 @@
+# API Definitions


Is "API Specification" a better fit?

See above comment about API Definition vs API Specification from Open API docs

api-definitions/README.md

docs/backend/api-definitions-guidelines.md

dismissing approval for the moment, as I want to make sure my approval reflects that i see consensus on the PR and there's some ongoing discussion. Thanks for engaging, everyone!

docs/backend/api-definitions-guidelines.md

relyt0925 · 2024-06-09T21:00:11Z

This looks good to me!

leseb

Thank you for your diligence in revisiting all the comments. We're making great progress, and I appreciate your efforts!

README.md

leseb · 2024-06-11T08:55:33Z

docs/backend/api-definitions-guidelines.md

+
+## Where will service API definitions live?
+
+Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:


Suggested change

Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:

Service API definitions will live a new repository `instructlab/service-api-definitions`. This repo will have two primary responsibilities:

I would prefer specification over definitions

@hickeyma, can you elaborate on that preference in light of the definition vs specification article? My thought is that this new repo would explicitly house definitions (for machine consumption). Are you thinking that it would also house specifications (for human consumption)?

Just to weigh in, the Kubernetes community often uses "spec" to define the API specification. I've seen "model" being used elsewhere. I'm ok with both TBH.

Ah, good point. I think you're right that the kubernetes community uses spec in this context a lot. I do think it's also a little muddy with CustomResourceDefinition which is the thing that defines the schema for a "thing" and then spec being the canonical name for the field within that schema where a user would create an instance of that "thing." I that vein, it still feels like Definition is the consistent word where the schema is defined.

That said, I'm a big fan of putting a Cares About number (1-10) on this kind of opinion and then just taking the highest one.

CA: 2

Looks like we went with instructlab/openai. I'm fine with that.

docs/backend/api-definitions-guidelines.md

hickeyma

Thanks for the updates @gabe-l-hart. Some more comments provided.

docs/backend/api-definitions-guidelines.md

README.md

hickeyma · 2024-06-11T11:36:01Z

docs/backend/api-definitions-guidelines.md

+
+## Where will service API definitions live?
+
+Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:


I would prefer specification over definitions

hickeyma · 2024-06-11T11:38:04Z

docs/backend/api-definitions-guidelines.md

+
+## Where will service API definitions live?
+
+Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:


Suggested change

Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:

Service API specifications will live a new repository `instructlab/service-api-specification`. This repo will have two primary responsibilities:

Same question as above. Why the preference for specification vs definition if the explicit goal of the repo is to own machine-consumable definitions. Are you thinking that the yaml format is the human-readable specification vs the generated language-specific packages are the machine-readable definitions?

Signed-off-by: Gabe Goodhart <[email protected]>

API and 'extensibilitiy' should be in the dictionary, but yaml -> YAML is reasonable. Signed-off-by: Gabe Goodhart <[email protected]>

Signed-off-by: Gabe Goodhart <[email protected]>

…repo Signed-off-by: Gabe Goodhart <[email protected]>

Co-authored-by: Martin Hickey <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

Signed-off-by: Gabe Goodhart <[email protected]>

docs/backend/api-definitions-guidelines.md

leseb

I think we are getting close, thanks for keeping up Gabe!

leseb · 2024-06-12T08:23:28Z

docs/backend/api-definitions-guidelines.md

+
+## Where will service API definitions live?
+
+Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:


Just to weigh in, the Kubernetes community often uses "spec" to define the API specification. I've seen "model" being used elsewhere. I'm ok with both TBH.

docs/backend/api-definitions-guidelines.md

…r repo name Signed-off-by: Gabe Goodhart <[email protected]>

Signed-off-by: Gabe Goodhart <[email protected]>

booxter · 2024-06-12T21:42:51Z

Directionally, this looks fine. The initial guidelines are quite abstract and perhaps will be clarified / filled with essence once actual APIs are proposed and actual bindings are generated. The main point is - the guidelines clarified that bindings will be auto-generated and hosted elsewhere; and there will be a separate repo for schema itself. This should be enough to merge it IMO.

leseb

I'm happy with the current content. The "how we sync generated APIs" many not yet be fully formed or may change a little as we begin to deep dive more into the implementation. However, I think we all agreed on:

Having a new repo for the source of truth (the API)
Generate client/SDK/server code using that API def onto new repository:
- it could be a pull from a client repo
- it can be a build/push from the API def repo

Thanks for all your efforts Gabe!

nathan-weinberg

one nit but otherwise LGTM

docs/backend/api-definitions-guidelines.md

Phrasing change for `openai` repo Co-authored-by: Nathan Weinberg <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

Signed-off-by: Martin Hickey <[email protected]>

hickeyma

Thanks for the updates @gabe-l-hart. Nearly there, just some small nits.

docs/backend/api-definitions-guidelines.md

Co-authored-by: Martin Hickey <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

hickeyma

This looks fine as it gives guidelines and direction with API definitions. I think the "how" will be worked out as we generate and sync specific APIs. We are taking the right approach using OpenAPI specifications and adding the specifications to a new repo for source of truth.

Thanks @gabe-l-hart for working on this.

hickeyma · 2024-07-03T08:44:26Z

@markstur @russellb The guideline looks good to go. Do you have any further comments or obejctions to merging and moving forward?

markstur

lgtm

gabe-l-hart force-pushed the ApiDefinitionGuidelines branch 2 times, most recently from fc7d51d to 27121d7 Compare May 31, 2024 21:42

russellb self-requested a review May 31, 2024 21:59

russellb added backend InstructLab Backend Services hold labels May 31, 2024

russellb removed the hold label Jun 4, 2024

russellb reviewed Jun 4, 2024

View reviewed changes

russellb mentioned this pull request Jun 4, 2024

mergify: automatically apply backend label #75

Merged

gabe-l-hart force-pushed the ApiDefinitionGuidelines branch from 27121d7 to 83b627c Compare June 6, 2024 19:58

russellb previously approved these changes Jun 6, 2024

View reviewed changes

gabe-l-hart mentioned this pull request Jun 6, 2024

Initial SDG REST API definitions #80

Draft

markstur reviewed Jun 6, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

leseb requested changes Jun 7, 2024

View reviewed changes

docs/backend/api-definitions-guidelines.md Outdated Show resolved Hide resolved

hickeyma requested changes Jun 7, 2024

View reviewed changes

booxter reviewed Jun 7, 2024

View reviewed changes

docs/backend/api-definitions-guidelines.md Show resolved Hide resolved

leseb requested changes Jun 11, 2024

View reviewed changes

hickeyma requested changes Jun 11, 2024

View reviewed changes

gabe-l-hart force-pushed the ApiDefinitionGuidelines branch from 2a76b78 to c13ab85 Compare June 11, 2024 17:38

gabe-l-hart added 6 commits June 11, 2024 11:52

ApiDefinitionGuidelines: Add README section for API Definitions

914fa3c

Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Add stub of api-definitions directory

0181d0d

Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Add api-definitions-guidelines doc

b02d87a

Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Fix spellcheck errors

88256c0

API and 'extensibilitiy' should be in the dictionary, but yaml -> YAML is reasonable. Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Add section on API versioning

dee4d60

Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Move API definitions under docs/backend

f32fbc5

Signed-off-by: Gabe Goodhart <[email protected]>

gabe-l-hart and others added 5 commits June 11, 2024 11:52

ApiDefinitionGuidelines: Expand on the casing guidelines

570697c

Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Adjust proposal to place API specs in a new …

18cdcf1

…repo Signed-off-by: Gabe Goodhart <[email protected]>

Improvements in language for OpenAPI

02b5a19

Co-authored-by: Martin Hickey <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Remove mention of api-definitions in this repo

20fb26e

Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Remove reference to local api-definitions dir

aba535b

Signed-off-by: Gabe Goodhart <[email protected]>

gabe-l-hart force-pushed the ApiDefinitionGuidelines branch from 08aefd1 to aba535b Compare June 11, 2024 17:52

booxter reviewed Jun 12, 2024

View reviewed changes

leseb reviewed Jun 12, 2024

View reviewed changes

gabe-l-hart added 2 commits June 12, 2024 10:26

ApiDefinitionGuidelines: update service-api-definitions -> openapi fo…

f67ffff

…r repo name Signed-off-by: Gabe Goodhart <[email protected]>

ApiDefinitionGuidelines: Remove third option for consuming APIs by copy

a90d70b

Signed-off-by: Gabe Goodhart <[email protected]>

leseb approved these changes Jun 13, 2024

View reviewed changes

nathan-weinberg approved these changes Jun 24, 2024

View reviewed changes

docs/backend/api-definitions-guidelines.md Outdated Show resolved Hide resolved

docs/backend/api-definitions-guidelines.md Show resolved Hide resolved

gabe-l-hart and others added 2 commits June 24, 2024 09:47

Update docs/backend/api-definitions-guidelines.md

e486715

Phrasing change for `openai` repo Co-authored-by: Nathan Weinberg <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

Merge branch 'main' into ApiDefinitionGuidelines

39579d0

Signed-off-by: Martin Hickey <[email protected]>

hickeyma self-requested a review July 2, 2024 15:42

hickeyma requested changes Jul 2, 2024

View reviewed changes

docs/backend/api-definitions-guidelines.md Outdated Show resolved Hide resolved

docs/backend/api-definitions-guidelines.md Outdated Show resolved Hide resolved

docs/backend/api-definitions-guidelines.md Outdated Show resolved Hide resolved

gabe-l-hart and others added 2 commits July 2, 2024 11:07

Typo fix in api definitions guidelines

12ae3a0

Co-authored-by: Martin Hickey <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

Wording improvements from review

5aff3e2

Co-authored-by: Martin Hickey <[email protected]> Signed-off-by: Gabe Goodhart <[email protected]>

hickeyma self-requested a review July 3, 2024 08:35

hickeyma approved these changes Jul 3, 2024

View reviewed changes

markstur approved these changes Jul 3, 2024

View reviewed changes

booxter mentioned this pull request Aug 7, 2024

Installation involves no less than 6 instructlab-* packages instructlab/instructlab#2010

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api definition guidelines #71

Api definition guidelines #71

gabe-l-hart commented May 31, 2024

russellb commented May 31, 2024

russellb commented May 31, 2024

gabe-l-hart commented Jun 3, 2024

russellb commented Jun 4, 2024

russellb left a comment

russellb left a comment

leseb left a comment

hickeyma left a comment

hickeyma Jun 7, 2024

gabe-l-hart Jun 10, 2024

relyt0925 commented Jun 9, 2024

leseb left a comment

leseb Jun 11, 2024

hickeyma Jun 11, 2024

gabe-l-hart Jun 11, 2024

leseb Jun 12, 2024

gabe-l-hart Jun 12, 2024 •

edited

Loading

leseb Jun 13, 2024

hickeyma left a comment

hickeyma Jun 11, 2024

hickeyma Jun 11, 2024

gabe-l-hart Jun 11, 2024

leseb left a comment

leseb Jun 12, 2024

booxter commented Jun 12, 2024

leseb left a comment •

edited

Loading

nathan-weinberg left a comment

hickeyma left a comment

hickeyma left a comment

hickeyma commented Jul 3, 2024

markstur left a comment


		## Where will service API definitions live?

		Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:

	Service API definitions will live a new repository github.com/instructlab/service-api-definitions. This repo will have two primary responsibilities:
	Service API specifications will live a new repository `instructlab/service-api-specification`. This repo will have two primary responsibilities:

Api definition guidelines #71

Are you sure you want to change the base?

Api definition guidelines #71

Conversation

gabe-l-hart commented May 31, 2024

Description

russellb commented May 31, 2024

russellb commented May 31, 2024

gabe-l-hart commented Jun 3, 2024

russellb commented Jun 4, 2024

russellb left a comment

Choose a reason for hiding this comment

russellb left a comment

Choose a reason for hiding this comment

leseb left a comment

Choose a reason for hiding this comment

hickeyma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relyt0925 commented Jun 9, 2024

leseb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabe-l-hart Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hickeyma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leseb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

booxter commented Jun 12, 2024

leseb left a comment • edited Loading

Choose a reason for hiding this comment

nathan-weinberg left a comment

Choose a reason for hiding this comment

hickeyma left a comment

Choose a reason for hiding this comment

hickeyma left a comment

Choose a reason for hiding this comment

hickeyma commented Jul 3, 2024

markstur left a comment

Choose a reason for hiding this comment

gabe-l-hart Jun 12, 2024 •

edited

Loading

leseb left a comment •

edited

Loading