chore: logins return Sessions #8883

wes-turner · 2024-02-23T23:20:06Z

Logins used to return an intermediate UsernameTokenPair that itself was used to create Sessions. We did it this way because the job of login is to find/create a valid token. But because login itself is a user-facing command, and tokens are an implementation detail most people don't care about, this patch updates the login API to just return a Session (the thing most users cared about in the first place).

If a user really wants to get a token, they can always access the token in the Session.

Additionally as a part of this patch, the UsernameTokenPair class has been removed -- it was kind of an in-between abstraction to make it easier for callers of login to create sessions.

This change also contains new BaseSession.with_retry to modify retries (by creating new sessions). We needed some mechanism for users to set retry to something other than the default, and passing retry into login would have been a weird interface (what does the retry of the eventual session have to do with logging in?).

MD-283

Description

Test Plan

Commentary (optional)

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

netlify · 2024-02-23T23:20:23Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`f6251f0`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65f9b5cd2ed2e20008152b01
😎 Deploy Preview	https://deploy-preview-8883--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.

codecov · 2024-02-23T23:22:54Z

Codecov Report

Attention: Patch coverage is 78.78788% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 47.75%. Comparing base (fa43bff) to head (f6251f0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8883      +/-   ##
==========================================
+ Coverage   47.74%   47.75%   +0.01%     
==========================================
  Files        1161     1161              
  Lines      143093   143070      -23     
  Branches     2372     2370       -2     
==========================================
+ Hits        68318    68330      +12     
+ Misses      74622    74587      -35     
  Partials      153      153

Flag	Coverage Δ
backend	`42.74% <ø> (+0.06%)`	⬆️
harness	`63.96% <78.78%> (+<0.01%)`	⬆️
web	`40.86% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
harness/determined/cli/_util.py	`68.08% <100.00%> (-0.67%)`	⬇️
harness/determined/common/api/__init__.py	`100.00% <100.00%> (ø)`
harness/determined/common/api/authentication.py	`85.44% <100.00%> (-0.33%)`	⬇️
...rness/determined/common/experimental/determined.py	`41.21% <100.00%> (-0.36%)`	⬇️
harness/determined/launch/deepspeed.py	`91.46% <100.00%> (-0.11%)`	⬇️
harness/determined/launch/horovod.py	`94.56% <100.00%> (-0.12%)`	⬇️
harness/tests/checkpoints/test_checkpoint.py	`100.00% <ø> (ø)`
harness/tests/cli/test_auth.py	`93.85% <100.00%> (+0.03%)`	⬆️
harness/tests/cli/test_experiment.py	`100.00% <100.00%> (ø)`
.../determined/common/experimental/test_checkpoint.py	`94.82% <100.00%> (-0.18%)`	⬇️
... and 16 more

... and 4 files with indirect coverage changes

rb-determined-ai

My name is @rb-determined-ai, and I approve of this change.

But also, what if UnauthSession had login and login_with_cache on it? That might be a more natural way to configure the retries.

Or it might be making the relationship between objects too complicated (that is how I felt, because I did consider that at some point).

But I wanted to voice it in case you hadn't thought of it and you loved the idea.

wes-turner · 2024-03-04T21:45:06Z

But also, what if UnauthSession had login and login_with_cache on it? That might be a more natural way to configure the retries.

Or it might be making the relationship between objects too complicated (that is how I felt, because I did consider that at some point).

But I wanted to voice it in case you hadn't thought of it and you loved the idea.

In code,

class UnauthSession(BaseSession):
  def login(self, user, password) -> Session:
    ...
  def login_with_cache(self, user, password) -> Session:
    ...

sess = api.UnauthSession(master, cert).login(user, password)

I like it. I don't know if I prefer it on the whole (makes creating a new session a little more awkward), but I do think it's a better abstraction to have implemented.

FWIW, I think adding an optional max_retries to UnauthSession.login is no better than max_retries on authentication.login, so in the case of the above I still prefer a BaseSession.with_retry.

rb-determined-ai · 2024-03-04T22:48:43Z

FWIW, I think adding an optional max_retries to UnauthSession.login is no better than max_retries on authentication.login, so in the case of the above I still prefer a BaseSession.with_retry.

I mean, I think you would put max_retries on UnauthSession not on its .login()

azhou-determined · 2024-03-05T20:14:19Z

i had originally thought login methods returning a session would be a great abstraction. the more i look at it/think about it, the less sure i am.

i think you're right that a parameter like retry shouldn't have anything to do with logging in. but then why should login methods return a session? seems like we're blurring abstractions somewhere. feels like a "pure" login method should return nothing (or at most a bool indicating whether login was successful or not). and if i keep with this train of thought, maybe logically all login should do is authenticate and persist the login token. and maybe get_current_session would be a separate method that returns the session.

i don't know how feasible this is, so take this however seriously you want. it's probably not worth the effort as i don't see it providing much value, and i think what you have is fine. just my $0.02.

harness/determined/common/api/authentication.py

harness/determined/common/api/_session.py

wes-turner · 2024-03-05T20:23:20Z

FWIW, I think adding an optional max_retries to UnauthSession.login is no better than max_retries on authentication.login, so in the case of the above I still prefer a BaseSession.with_retry.

I mean, I think you would put max_retries on UnauthSession not on its .login()

Oh... Yeah. That's better.

azhou-determined · 2024-03-07T00:43:59Z

harness/determined/common/api/authentication.py

+    Unlike ``login``, this function may not send a login request to the master. It will instead
+    first attempt to find a valid token in the TokenStore, and only if that fails will it post a
+    login request to the master to generate a new one. As with ``login``, the token is then baked
+    into a new api.Session object to sign future communication with master.


quotes api.Session

Fixed. Thanks!

azhou-determined

lgtm

wes-turner · 2024-03-14T23:47:17Z

But also, what if UnauthSession had login and login_with_cache on it? That might be a more natural way to configure the retries.

I do like this idea. A lot. But I'm not sure how much more I like it than what's implemented, and keeping in mind ROI I don't think it's worth working out.

wes-turner · 2024-03-15T22:31:40Z

master/static/srv/check_idle.py

@@ -75,8 +75,7 @@ def main():
    notebook_server = f"https://127.0.0.1:{port}/proxy/{notebook_id}"
    master_url = api.canonicalize_master_url(os.environ["DET_MASTER"])
    cert = certs.default_load(master_url)
-    utp = authentication.login_with_cache(info.master_url, cert=cert)
-    sess = api.Session(master_url, utp, cert)
+    sess = authentication.login_with_cache(master_url, cert=cert)


Unrelated to the change; this looks like a bugfix. info had not been in scope.

Decided to not implement a regression test. This doesn't seem like functionality we care much about.

NicholasBlaskey · 2024-03-18T13:09:20Z

harness/determined/common/api/_session.py

 import json as _json
-from typing import Any, Dict, Optional, Tuple, Union
+from typing import Any, Dict, Optional, Tuple, TypeVar, Union

 import requests
 import urllib3



I think this will also need to be updated

determined/.circleci/scripts/wait_for_perf_migration_upload_results.py

Line 38 in bc1b431

utp = authentication.login("http://127.0.0.1:8080", "admin", "", cert)

Fixed. I hadn't known that **/*.py glob skipped hidden directories. Thanks for catching this.

loksonarius

Approving CI changes on behalf of infra

Used to return an intermediate `UsernameTokenPair` that itself was used for sessions. This class has been removed. Also contains new `BaseSession.with_retry` to modify retries (by creating new sessions).

Also includes a bugfix in `check_idle.py`, I think, where master wasn't being propagated.

Logins used to return an intermediate UsernameTokenPair that itself was used to create Sessions. We did it this way because the job of login is to find/create a valid token. But because login itself is a user-facing command, and tokens are an implementation detail most people don't care about, this patch updates the login API to just return a Session (the thing most users cared about in the first place). If a user really wants to get a token, they can always access the token in the Session. Additionally as a part of this patch, the UsernameTokenPair class has been removed -- it was kind of an in-between abstraction to make it easier for callers of login to create sessions. This change also contains new BaseSession.with_retry to modify retries (by creating new sessions). We needed some mechanism for users to set retry to something other than the default, and passing retry into login would have been a weird interface (what does the retry of the eventual session have to do with logging in?).

cla-bot bot added the cla-signed label Feb 23, 2024

wes-turner force-pushed the wes/login-returns-session branch 3 times, most recently from 9dc6474 to 3da73c1 Compare March 1, 2024 21:24

wes-turner force-pushed the wes/login-returns-session branch from 3da73c1 to ba344f7 Compare March 4, 2024 19:30

wes-turner requested review from rb-determined-ai and azhou-determined March 4, 2024 20:05

wes-turner marked this pull request as ready for review March 4, 2024 20:05

wes-turner requested a review from a team as a code owner March 4, 2024 20:05

rb-determined-ai approved these changes Mar 4, 2024

View reviewed changes

azhou-determined reviewed Mar 5, 2024

View reviewed changes

harness/determined/common/api/authentication.py Show resolved Hide resolved

harness/determined/common/api/authentication.py Show resolved Hide resolved

harness/determined/common/api/_session.py Show resolved Hide resolved

harness/determined/common/api/_session.py Show resolved Hide resolved

azhou-determined reviewed Mar 7, 2024

View reviewed changes

azhou-determined approved these changes Mar 7, 2024

View reviewed changes

wes-turner requested a review from a team as a code owner March 15, 2024 22:29

wes-turner requested a review from NicholasBlaskey March 15, 2024 22:29

wes-turner commented Mar 15, 2024

View reviewed changes

wes-turner force-pushed the wes/login-returns-session branch from 5320a7a to db781d5 Compare March 16, 2024 06:08

NicholasBlaskey approved these changes Mar 18, 2024

View reviewed changes

wes-turner requested review from a team as code owners March 18, 2024 17:05

wes-turner requested review from gt2345 and loksonarius March 18, 2024 17:05

loksonarius approved these changes Mar 18, 2024

View reviewed changes

wes-turner removed the request for review from gt2345 March 18, 2024 18:18

wes-turner force-pushed the wes/login-returns-session branch from 055c190 to 9bab34b Compare March 18, 2024 19:09

wes-turner added 9 commits March 19, 2024 08:56

chore: logins return Sessions

4d1c932

Used to return an intermediate `UsernameTokenPair` that itself was used for sessions. This class has been removed. Also contains new `BaseSession.with_retry` to modify retries (by creating new sessions).

Deepspeed test mocks

c468abe

test_horovod fix

e6d71ce

fix from exec.launch

1082eb2

docstring update for login_with_cache

dff4341

Additional documentation for login/logout

a168ca2

quotes in docstring

62075d0

additional places I'd missed that used UTPs

95e8652

Also includes a bugfix in `check_idle.py`, I think, where master wasn't being propagated.

Another session fix in a non-python directory

f6251f0

wes-turner force-pushed the wes/login-returns-session branch from 9bab34b to f6251f0 Compare March 19, 2024 15:56

wes-turner merged commit a603f4c into main Mar 19, 2024
103 checks passed

wes-turner deleted the wes/login-returns-session branch March 19, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: logins return Sessions #8883

chore: logins return Sessions #8883

wes-turner commented Feb 23, 2024 •

edited by jira bot

Loading

netlify bot commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading

rb-determined-ai left a comment

wes-turner commented Mar 4, 2024 •

edited

Loading

rb-determined-ai commented Mar 4, 2024 •

edited

Loading

azhou-determined commented Mar 5, 2024

wes-turner commented Mar 5, 2024

azhou-determined Mar 7, 2024 •

edited

Loading

wes-turner Mar 14, 2024

azhou-determined left a comment

wes-turner commented Mar 14, 2024

wes-turner Mar 15, 2024 •

edited

Loading

wes-turner Mar 18, 2024

NicholasBlaskey Mar 18, 2024

wes-turner Mar 18, 2024

loksonarius left a comment

chore: logins return Sessions #8883

chore: logins return Sessions #8883

Conversation

wes-turner commented Feb 23, 2024 • edited by jira bot Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Feb 23, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

rb-determined-ai left a comment

Choose a reason for hiding this comment

wes-turner commented Mar 4, 2024 • edited Loading

rb-determined-ai commented Mar 4, 2024 • edited Loading

azhou-determined commented Mar 5, 2024

wes-turner commented Mar 5, 2024

azhou-determined Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

wes-turner Mar 14, 2024

Choose a reason for hiding this comment

azhou-determined left a comment

Choose a reason for hiding this comment

wes-turner commented Mar 14, 2024

wes-turner Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

wes-turner Mar 18, 2024

Choose a reason for hiding this comment

NicholasBlaskey Mar 18, 2024

Choose a reason for hiding this comment

wes-turner Mar 18, 2024

Choose a reason for hiding this comment

loksonarius left a comment

Choose a reason for hiding this comment

wes-turner commented Feb 23, 2024 •

edited by jira bot

Loading

netlify bot commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading

wes-turner commented Mar 4, 2024 •

edited

Loading

rb-determined-ai commented Mar 4, 2024 •

edited

Loading

azhou-determined Mar 7, 2024 •

edited

Loading

wes-turner Mar 15, 2024 •

edited

Loading