feat: generate and use swagger typescript client [DET-3249 DET-3324 DET-3355] #691

hamidzr · 2020-06-11T00:26:31Z

Description

depends on #680

Test Plan

make sure we can build the npm module: a java step and a Node step.
make sure WebUI builds and we can use a generated endpoint in React (logout)
- Click the logout to test logout and check that the cookie is invalid
make sure CI builds in a reasonable time
check and make sure types and API comments come through the generated TS code.

Commentary (optional)

webui/react/src/pages/SignOut.tsx

hamidzr · 2020-06-13T00:48:31Z

for later: it seems proto get-deps does not get all the dependencies for that module https://app.circleci.com/pipelines/github/determined-ai/determined/2476/workflows/b5b68796-3f99-4d58-801a-e6d915804251/jobs/50858

webui/react/tsconfig.json

webui/react/src/services/api.ts

hkang1

The use case of the swagger api looks good for the logout example. We can keep evaluating other use cases, such as a GET with params, POST and STREAMING (logs) as they get implemented.

justin-determined-ai

Great work! I think this could use a little more detail on how you've tested this change, and perhaps talk a bit about how we expect the new package to be versioned/maintained/published.

justin-determined-ai · 2020-06-18T18:57:10Z

webui/react/Makefile

+	# WARN this module also depends on the swagger generated api client
+	# which is not built or updated here


question: do we want to get rid of this/improve this in future work, or is it just a caution for other people in this code? if the former, can we make a ticket/link it; if the latter, can we be a bit more clear on where one should go look if they want more information?

hmm I don't see a good way of getting rid of this without more changes to our build system or introducing separate make targets and using different ones in CI vs isolated WebUI development vs local cluster build. I can add more information

added wording to point the user to the target module. the info here would/could change if we make changes to the CI. I designed it this way to break up CI dependencies. if we don't want to do the process would be simpler but slower.

justin-determined-ai · 2020-06-18T18:58:17Z

webui/api-ts-sdk/package.json

@@ -0,0 +1,20 @@
+{
+  "name": "@determined-ai/api-ts-sdk",
+  "version": "1.0.0",


blocking: Can we make sure we've considered what we want our numbering scheme to look like here? For example, should it map directly to our API version? (If so, maybe we start at 0.0.0 for the moment, until we're ready to declare API v1 "done"?)

sidenote the version probably wouldn't really matter until we start publishing the package on NPM. we could probably defer it until we come up with fine grain versioning for our APIs

I updated the version

hamidzr · 2020-06-18T22:14:12Z

any thoughts on the package name for the API ts sdk/binding ?

webui/api-ts-sdk/.gitignore

justin-determined-ai

LGTM, thanks!

justin-determined-ai · 2020-06-25T20:08:23Z

CONTRIBUTING.md

 - Docker (>= 19.03)
 - Protoc (>= 3.0)
+- Java (>= 7)


question: Do we need to announce in engineering once this lands that they need to update? Is this just if you want to work on the front-end?

hmm needed by anyone that needs to build the npm package, publish it, or otherwise use it so for the time being mostly anyone working touching the frontend or wanting to run frontend tests.

Please announce in #engineering once this lands. Thanks!

justin-determined-ai · 2020-06-25T20:09:43Z

webui/react/src/services/api.ts

@@ -1,14 +1,21 @@
 /* eslint-disable @typescript-eslint/camelcase */
+import * as DetSwagger from '@determined-ai/api-ts-sdk';


non-blocking: Feels weird to call out the technology as part of the variable name but I don't have a better idea so this is probably fine.

I agree I'm open to suggestions. DetApi makes the most sense to me but we already have something like that. IMO once we are mostly on the swagger generated we'd switch it around 🤔

Yup, please don't block on this.

…-ai#691) The expconf resources.max_slots attribute is documented as managed by Slurm, but there was a partial (non-functional) implementation in DispatchRM as well. If we ever submitted max_slots worth of work the remainder would remain QUEUED forever. I did attempt to fix the existing support, but the obvious fixes were not sufficient. Since this is already documented as managed by Slurm, just remove the support and let the workload managers manage it.

The expconf resources.max_slots attribute is documented as managed by Slurm, but there was a partial (non-functional) implementation in DispatchRM as well. If we ever submitted max_slots worth of work the remainder would remain QUEUED forever. I did attempt to fix the existing support, but the obvious fixes were not sufficient. Since this is already documented as managed by Slurm, just remove the support and let the workload managers manage it.

…-ai#691) The expconf resources.max_slots attribute is documented as managed by Slurm, but there was a partial (non-functional) implementation in DispatchRM as well. If we ever submitted max_slots worth of work the remainder would remain QUEUED forever. I did attempt to fix the existing support, but the obvious fixes were not sufficient. Since this is already documented as managed by Slurm, just remove the support and let the workload managers manage it.

hamidzr requested review from hkang1 and ybt195 June 11, 2020 00:26

hamidzr assigned hkang1 Jun 11, 2020

cla-bot bot added the cla-signed label Jun 11, 2020

hamidzr commented Jun 11, 2020

View reviewed changes

webui/react/src/pages/SignOut.tsx Show resolved Hide resolved

hamidzr force-pushed the 3249-swagger-ts-client branch 2 times, most recently from 82cd024 to c5985bf Compare June 11, 2020 03:20

hamidzr added the Work in progress label Jun 12, 2020

hamidzr unassigned hkang1 Jun 12, 2020

hamidzr marked this pull request as draft June 12, 2020 21:23

hamidzr removed the Work in progress label Jun 12, 2020

hamidzr force-pushed the 3249-swagger-ts-client branch 3 times, most recently from 4799c98 to 0ac4a59 Compare June 13, 2020 00:33

hamidzr force-pushed the 3249-swagger-ts-client branch from f5fc3f2 to c33ce3f Compare June 13, 2020 00:49

hamidzr assigned hkang1 and ybt195 Jun 13, 2020

hamidzr marked this pull request as ready for review June 13, 2020 01:36

hamidzr force-pushed the 3249-swagger-ts-client branch 3 times, most recently from e91da6e to 2644c18 Compare June 13, 2020 01:40

hamidzr changed the title ~~feat: generate and use swagger typescript client [DET-3249]~~ feat: generate and use swagger typescript client [DET-3249 DET-3224 DET-3355] Jun 13, 2020

hamidzr changed the title ~~feat: generate and use swagger typescript client [DET-3249 DET-3224 DET-3355]~~ feat: generate and use swagger typescript client [DET-3249 DET-3324 DET-3355] Jun 13, 2020

hkang1 reviewed Jun 15, 2020

View reviewed changes

webui/react/tsconfig.json Show resolved Hide resolved

hkang1 reviewed Jun 16, 2020

View reviewed changes

webui/react/src/services/api.ts Show resolved Hide resolved

hkang1 approved these changes Jun 16, 2020

View reviewed changes

hkang1 assigned ybt195 and unassigned hkang1 and ybt195 Jun 16, 2020

hamidzr requested a review from justin-determined-ai June 18, 2020 18:49

justin-determined-ai reviewed Jun 18, 2020

View reviewed changes

hamidzr added 4 commits June 22, 2020 14:13

add Java to CONTRIBUTING.md

b982b93

ci: circle ci

d8aed40

feat: build swagger client and use it

55f5cdd

fix import typo

8cdf9c4

hamidzr force-pushed the 3249-swagger-ts-client branch from 5170640 to 8cdf9c4 Compare June 22, 2020 21:30

hamidzr assigned justin-determined-ai Jun 22, 2020

hamidzr commented Jun 24, 2020

View reviewed changes

webui/api-ts-sdk/.gitignore Show resolved Hide resolved

hamidzr added 2 commits June 24, 2020 13:41

remove added generated code under src/

f419010

add a default make target

2533830

justin-determined-ai approved these changes Jun 25, 2020

View reviewed changes

hamidzr merged commit 06d0850 into determined-ai:master Jun 25, 2020

hamidzr deleted the 3249-swagger-ts-client branch June 25, 2020 22:46

dannysauer added this to the 0.12.11 milestone Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generate and use swagger typescript client [DET-3249 DET-3324 DET-3355] #691

feat: generate and use swagger typescript client [DET-3249 DET-3324 DET-3355] #691

hamidzr commented Jun 11, 2020 •

edited

Loading

hamidzr commented Jun 13, 2020

hkang1 left a comment

justin-determined-ai left a comment

justin-determined-ai Jun 18, 2020

hamidzr Jun 18, 2020 •

edited

Loading

hamidzr Jun 22, 2020

justin-determined-ai Jun 18, 2020

hamidzr Jun 18, 2020 •

edited

Loading

hamidzr Jun 22, 2020

hamidzr commented Jun 18, 2020

justin-determined-ai left a comment

justin-determined-ai Jun 25, 2020

hamidzr Jun 25, 2020

justin-determined-ai Jun 25, 2020

justin-determined-ai Jun 25, 2020

hamidzr Jun 25, 2020

justin-determined-ai Jun 25, 2020

		# WARN this module also depends on the swagger generated api client
		# which is not built or updated here

		@@ -1,14 +1,21 @@
		/* eslint-disable @typescript-eslint/camelcase */
		import * as DetSwagger from '@determined-ai/api-ts-sdk';

feat: generate and use swagger typescript client [DET-3249 DET-3324 DET-3355] #691

feat: generate and use swagger typescript client [DET-3249 DET-3324 DET-3355] #691

Conversation

hamidzr commented Jun 11, 2020 • edited Loading

Description

Test Plan

Commentary (optional)

hamidzr commented Jun 13, 2020

hkang1 left a comment

Choose a reason for hiding this comment

justin-determined-ai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidzr Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidzr Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidzr commented Jun 18, 2020

justin-determined-ai 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidzr commented Jun 11, 2020 •

edited

Loading

hamidzr Jun 18, 2020 •

edited

Loading

hamidzr Jun 18, 2020 •

edited

Loading