fix: cli is not a library! #7891

rb-determined-ai · 2023-09-14T19:34:20Z

Direct calls into the CLI are problematic, because the CLI uses a variety of authentication techniques, some of which are only configured by the cli's main() codepath. That means that cli auth and cert singletons have been configured all over e2e tests in order to support direct CLI calls.

Avoiding direct calls to cli functions is a step in the direction of cleaning up our authentication story.

netlify · 2023-09-14T19:34:26Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`bdb6efe`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65494ea46305df00083c6e70
😎 Deploy Preview	https://deploy-preview-7891--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

harness/determined/common/api/_util.py

hamidzr

looks good I don't have much to add here I'll let mlsys review unless there is a specific part you want me to look at

hamidzr · 2023-09-18T20:58:51Z

e2e_tests/tests/cluster/test_logging.py

@@ -63,10 +62,10 @@ def test_trial_logs() -> None:
 @pytest.mark.parametrize(
    "task_type,task_config,log_regex",
    [
-        (command.TaskTypeCommand, {"entrypoint": ["echo", "hello"]}, re.compile("^.*hello.*$")),


because the CLI is not a library. It should never be imported from outside the cli/ directory.

Importing it for a few static strings is a violation of the application-nature of the CLI, with no particular benefit. I don't think these strings are going to change in the next 10 years.

I didn't check where the command namespace was coming from. makes sense.

wes-turner

Reviewing the relocation of functionality from the CLI to determined.common.api. Not reviewing its contents, which I think are out of scope for this PR.

LGTM. Thanks for making this better.

harness/determined/common/api/_util.py

wes-turner · 2023-09-20T00:22:47Z

e2e_tests/tests/cluster/test_rbac.py

+        user = bindings.v1User(username=api_utils.get_random_string(), admin=False, active=True)
+        creds = api_utils.create_test_user(True, user=user)
+        for role in roles:
+            api.assign_role(


Excision of determined.cli.rbac.assign_role into determined.common.api.assign_role looks great, as does its use here. api.assign_role seems like a pretty reasonable place to call something from that fronts the backend. But there's something about it living in determined.common.api._util that feels wrong to me. It's a nit either way, and who really cares what file the function is in, but if you think that it'll be helpful to us to someday have structure to that directory, I think now is a good time to start it.

I really don't care what file the function is in. If you want me to put it in a different file, lmk what to name it and I'll do it.

I do like having it in the api namespace, and this is a case where I really like the separation between how code is called and where code is defined.

I do like having it in the api namespace, and this is a case where I really like the separation between how code is called and where code is defined.

I've not had cause to differentiate the two in the past, but I agree. It's really nice, here.

This comment applies to many of the other moved functions, too. How about determined/common/api/_rbac.py?

I think that sounds like a great name.

I put all the functions I added there, since they're all rbac-related. Even not_found_errs is rbac-related.

👍 I like it.

wes-turner · 2023-09-20T00:57:55Z

harness/determined/common/api/_util.py

+    return bindings.get_GetWorkspace(session, id=w[0].id).workspace
+
+
+# not_found_errs mirrors NotFoundErrs from the golang api/errors.go. In the cases where


I don't get what's going on with this function. Could it be part of your cleanup?

Yes, I'll look into it. I didn't ask that question earlier, I just shuffled it around.

looked into it. It just helps users remember that 404 errors in rbac cases might be permissions errors.

I promoted the comment to a docstring and added more explanation.

The docstring is a better place to put the help text, for sure. And thanks for figuring out what the function is for.

I still have to do more work to understand what the function does than I want a person to have to do. Something seems off about the design. Bleh.

But anyway, I think bigger changes are outside the scope of this PR. So maybe here just a suggestion like:

"""Construct a NotFoundException from passed strings. This function creates NotFoundExceptions with the same syntax as the NotFoundErrs from api/errors.go so that messages from both layers look the same. If RBAC is enabled, the exceptions underlying the 404s that this function constructs might be the result of a permissions error. In that event, this function appends a suggestion to the constructed exception string that the user to check permissions. """

The type mismatch between the actual return type and its superclass is a little weird. That'd be an easy fix if you want to make it.

wes-turner · 2023-09-28T23:51:58Z

harness/determined/common/api/_rbac.py

+    group_name_to_assign: Optional[str] = None,
+) -> Tuple[List[bindings.v1UserRoleAssignment], List[bindings.v1GroupRoleAssignment]]:
+    if (username_to_assign is None) == (group_name_to_assign is None):
+        # XXX: very weird that we use api.errors.BadRequestException here!


Don't forget to either remove me or turn this into a TODO and/or ticket.

ah yes, thank you

stoksc

backend lgtm as long as tests pass

rb-determined-ai · 2023-10-06T22:50:42Z

status update: this is blocked on bringing the EE changes in e2e_tests into the OSS repo.

We can't solve the rebase problem for the whole world, but we can solve it for this directory!

rb-determined-ai · 2023-11-02T22:37:37Z

status update: EE changes to e2e_tests are finally in OSS now, and this PR can proceed as planned.

wes-turner · 2023-11-02T23:47:43Z

harness/determined/common/api/_rbac.py

+    raise api.errors.BadRequestException(f"could not find role name {role_name}")
+
+
+def create_assignment_request(


I don't like this method. It's got all kinds of weird. It's not your job, but do you want to be the person on the other side of the git blame?

I think it should be split into:

create_user_assignment_request(session, role, workspace, username) -> bindings.v1UserRoleAssignment create_group_assignment_request(session, role, workspace, group) -> bindings.v1GroupRoleAssignment

lol, you are exactly right. I didn't even read the function body.

wes-turner

New changes yes.

Direct calls into the CLI are problematic, because the CLI uses a variety of authentication techniques, some of which are only configured by the cli's __main__() codepath. That means that cli auth and cert singletons have been configured all over e2e tests in order to support direct CLI calls. Avoiding direct calls to cli functions is a step in the direction of cleaning up our authentication story.

rb-determined-ai · 2023-11-06T20:43:52Z

I removed a review request from the web team, seems to have been from an old force-push-to-master incident.

rb-determined-ai · 2023-11-06T21:23:10Z

Everything passed except some known flakes. Same story over in the EE repo. Force-merging.

rb-determined-ai requested review from wes-turner and hamidzr September 14, 2023 19:34

rb-determined-ai assigned wes-turner Sep 14, 2023

rb-determined-ai requested a review from a team as a code owner September 14, 2023 19:34

cla-bot bot added the cla-signed label Sep 14, 2023

rb-determined-ai commented Sep 14, 2023

View reviewed changes

harness/determined/common/api/_util.py Outdated Show resolved Hide resolved

rb-determined-ai force-pushed the rb/cli-not-a-library branch 3 times, most recently from 591eedc to 556d2e9 Compare September 18, 2023 18:17

hamidzr reviewed Sep 18, 2023

View reviewed changes

rb-determined-ai force-pushed the rb/cli-not-a-library branch 2 times, most recently from 92b36be to fc341b8 Compare September 19, 2023 15:23

wes-turner approved these changes Sep 20, 2023

View reviewed changes

carolinaecalderon force-pushed the main branch from 9ac8f82 to 8eff8ac Compare September 20, 2023 15:14

carolinaecalderon requested review from a team as code owners September 20, 2023 15:14

carolinaecalderon requested review from ClaireNeveu and dzhu September 20, 2023 15:14

dzhu removed request for dzhu and a team September 25, 2023 15:46

rb-determined-ai force-pushed the rb/cli-not-a-library branch from fc341b8 to f0256ab Compare September 28, 2023 23:06

wes-turner reviewed Sep 28, 2023

View reviewed changes

rb-determined-ai force-pushed the rb/cli-not-a-library branch 2 times, most recently from b38bfa3 to 5d3a5ba Compare September 29, 2023 19:32

stoksc approved these changes Oct 4, 2023

View reviewed changes

rb-determined-ai force-pushed the rb/cli-not-a-library branch from 5d3a5ba to da8090a Compare November 2, 2023 22:36

rb-determined-ai force-pushed the rb/cli-not-a-library branch from da8090a to d916469 Compare November 2, 2023 23:18

wes-turner reviewed Nov 2, 2023

View reviewed changes

rb-determined-ai changed the title ~~fix: cli is not a library!~~ fix: cli is not a library! Nov 3, 2023

wes-turner approved these changes Nov 3, 2023

View reviewed changes

rb-determined-ai force-pushed the rb/cli-not-a-library branch from d916469 to 5d463de Compare November 6, 2023 17:57

rb-determined-ai force-pushed the rb/cli-not-a-library branch from 5d463de to bdb6efe Compare November 6, 2023 20:37

rb-determined-ai removed request for a team and ClaireNeveu November 6, 2023 20:42

rb-determined-ai merged commit af995ba into main Nov 6, 2023
68 of 80 checks passed

rb-determined-ai deleted the rb/cli-not-a-library branch November 6, 2023 21:23

dannysauer added this to the 0.26.4 milestone Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cli is not a library! #7891

fix: cli is not a library! #7891

rb-determined-ai commented Sep 14, 2023

netlify bot commented Sep 14, 2023 •

edited

Loading

hamidzr left a comment

hamidzr Sep 18, 2023

rb-determined-ai Sep 18, 2023

hamidzr Sep 19, 2023

wes-turner left a comment •

edited

Loading

wes-turner Sep 20, 2023

rb-determined-ai Sep 20, 2023

wes-turner Sep 20, 2023

rb-determined-ai Sep 28, 2023

wes-turner Sep 28, 2023

wes-turner Sep 20, 2023

rb-determined-ai Sep 20, 2023

rb-determined-ai Sep 28, 2023

wes-turner Sep 28, 2023 •

edited

Loading

wes-turner Sep 28, 2023

rb-determined-ai Sep 28, 2023

stoksc left a comment

rb-determined-ai commented Oct 6, 2023

rb-determined-ai commented Nov 2, 2023

wes-turner Nov 2, 2023

rb-determined-ai Nov 3, 2023

wes-turner left a comment

rb-determined-ai commented Nov 6, 2023

rb-determined-ai commented Nov 6, 2023

		return bindings.get_GetWorkspace(session, id=w[0].id).workspace


		# not_found_errs mirrors NotFoundErrs from the golang api/errors.go. In the cases where

		raise api.errors.BadRequestException(f"could not find role name {role_name}")


		def create_assignment_request(

fix: cli is not a library! #7891

fix: cli is not a library! #7891

Conversation

rb-determined-ai commented Sep 14, 2023

netlify bot commented Sep 14, 2023 • edited Loading

✅ Deploy Preview for determined-ui ready!

hamidzr 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

wes-turner left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wes-turner Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoksc left a comment

Choose a reason for hiding this comment

rb-determined-ai commented Oct 6, 2023

rb-determined-ai commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wes-turner left a comment

Choose a reason for hiding this comment

rb-determined-ai commented Nov 6, 2023

rb-determined-ai commented Nov 6, 2023

netlify bot commented Sep 14, 2023 •

edited

Loading

wes-turner left a comment •

edited

Loading

wes-turner Sep 28, 2023 •

edited

Loading