[WB-6865] ensure captive key input even if user presses enter #2721

dannygoldstein · 2021-10-03T22:41:11Z

https://wandb.atlassian.net/browse/WB-6865

Description

Before if someone did wandb login --relogin then hit enter without entering anything, the api key prompt would vanish and be a no-op:

(wandb-3.7) danielgoldstein@Daniels-MBP ~ % wandb login --relogin
wandb: You can find your API key in your browser here: https://wandb.ai/authorize
wandb: Paste an API key from your profile and hit enter: [enter]
(wandb-3.7) danielgoldstein@Daniels-MBP ~ %

Now if you hit enter without entering anything, it will reprompt you for an API key, or tell you how to quit out and do a no-op:

(wandb-3.7) danielgoldstein@Daniels-MBP ~ % wandb login --relogin
wandb: You can find your API key in your browser here: https://wandb.ai/authorize
wandb: Paste an API key from your profile and hit enter, or press ctrl+c to quit: [enter]
wandb: ERROR No API key specified. Please specify an API key
wandb: You can find your API key in your browser here: https://wandb.ai/authorize
wandb: Paste an API key from your profile and hit enter, or press ctrl+c to quit:

Manually

Release Notes

Below, please enter user-facing release notes as one or more bullet points.
If your change is not user-visible, write NO RELEASE NOTES instead, with no bullet points.

------------- BEGIN RELEASE NOTES ------------------
Ensures API key prompt remains captive when user enters nothing
------------- END RELEASE NOTES --------------------

codecov · 2021-10-03T22:55:31Z

Codecov Report

Merging #2721 (28e2e93) into master (7e48437) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2721      +/-   ##
==========================================
+ Coverage   77.18%   77.19%   +0.01%     
==========================================
  Files         178      178              
  Lines       26619    26622       +3     
==========================================
+ Hits        20546    20551       +5     
+ Misses       6073     6071       -2

Flag	Coverage Δ
functest	`53.00% <38.88%> (+0.06%)`	⬆️
unittest	`70.24% <100.00%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
wandb/sdk/lib/apikey.py	`87.78% <100.00%> (-2.14%)`	⬇️
wandb/sdk/wandb_login.py	`94.77% <100.00%> (+1.35%)`	⬆️
wandb/sdk/launch/runner/abstract.py	`68.91% <0.00%> (-1.36%)`	⬇️
wandb/sdk/wandb_run.py	`87.29% <0.00%> (-0.27%)`	⬇️
wandb/sdk/lib/git.py	`75.86% <0.00%> (ø)`
wandb/sdk/launch/runner/local.py	`75.00% <0.00%> (ø)`
wandb/sdk/internal/sender.py	`91.92% <0.00%> (+0.13%)`	⬆️
wandb/sdk/internal/meta.py	`90.18% <0.00%> (+3.06%)`	⬆️
wandb/sdk/launch/launch.py	`88.88% <0.00%> (+3.70%)`	⬆️

raubitsj · 2021-10-11T19:11:31Z

I think there is a problem with the PR as the tests are failing.

Also this pattern needs to be fixed (i think it came in the last PR in this area):

in wandb_login.py:

    def _prompt_api_key(self) -> Tuple[Optional[str], ApiKeyStatus]:

        api = Api(self._settings)
        try:
            key = apikey.prompt_api_key(
                self._settings,
                api=api,
                no_offline=self._settings.force if self._settings else None,
                no_create=self._settings.force if self._settings else None,
            )
        except ValueError as e:
            # invalid key provided, try again
            wandb.termerror(e.args[0])
            return self._prompt_api_key()

recursive exception handling seems like something to avoid.

dannygoldstein · 2021-10-21T00:34:02Z

I think there is a problem with the PR as the tests are failing.

Fixed this by explicitly handling LOGIN_CHOICE_DRYRUN

Also this pattern needs to be fixed (i think it came in the last PR in this area):

in wandb_login.py:

    def _prompt_api_key(self) -> Tuple[Optional[str], ApiKeyStatus]:

        api = Api(self._settings)
        try:
            key = apikey.prompt_api_key(
                self._settings,
                api=api,
                no_offline=self._settings.force if self._settings else None,
                no_create=self._settings.force if self._settings else None,
            )
        except ValueError as e:
            # invalid key provided, try again
            wandb.termerror(e.args[0])
            return self._prompt_api_key()

recursive exception handling seems like something to avoid.

removed recursive exception handling and replaced with a loop

dannygoldstein · 2021-10-21T16:50:04Z

added a test to improve coverage

vwrj · 2021-11-04T23:25:56Z

It goes through the "choices" flow again after pressing [enter] instead of the API key line.

dannygoldstein · 2021-11-07T21:44:30Z

@vwrj I think going through the flow in this context again is fine,

it's just a bit confusing given that the error message asks the user to immediately re-specify the API key. I updated the error message to not set that expectation. Aside from that I think the logic / flow of re-prompting for choices at that stage is ok

raubitsj · 2021-11-10T00:08:44Z

wandb/sdk/wandb_login.py

+            if not key:
+                return None, ApiKeyStatus.OFFLINE
+            else:
+                return key, ApiKeyStatus.VALID


i think this was better the way it was... else is strange here and not needed

removed else

raubitsj · 2021-11-10T00:16:05Z

wandb/sdk/lib/apikey.py

@@ -200,7 +205,7 @@ def write_netrc(host, entity, key):

 def write_key(settings, key, api=None, anonymous=False):
    if not key:
-        return
+        raise ValueError("No API key specified.")


as long as you have gone through all the paths that could call this and they make sense... raising a exception seems like a much better thing to do, im just worried there is some path that is calling this with an empty key and expects it to pass

all the paths are guarded. there are 6 in the code:

configure_api_key() in _WandbLogin() which only is called if key is not None,

and 5 in prompt_api_key, which is called only in one place and in that one place has an exception handler (implemented in this PR).

dannygoldstein and others added 3 commits September 30, 2021 10:21

raise ValueError on empty API key in write_key

ba7b5d6

add ctrl+c message

289d0c3

Merge branch 'master' into api-key-hotfix

9b64506

dannygoldstein requested a review from raubitsj October 3, 2021 22:41

dannygoldstein added 3 commits October 20, 2021 15:27

Merge branch 'master' into api-key-hotfix

63aefc0

remove anti-pattern: recursive exception handling

f10807b

ensure correct behavior when we choose to run w&b in offline mode

b3d0afa

dannygoldstein requested review from raubitsj and removed request for raubitsj October 21, 2021 00:34

add test

f65e94a

dannygoldstein and others added 4 commits October 21, 2021 09:51

remove ipdb from requirements

967ab32

rename test

9a5a940

run black

ff9ec91

Merge branch 'master' into api-key-hotfix

c0bed2c

Merge branch 'master' into api-key-hotfix

0aeb43b

don't ask for another api key yet

28e8896

dannygoldstein force-pushed the api-key-hotfix branch from 23e4407 to 28e8896 Compare November 7, 2021 21:53

raubitsj reviewed Nov 10, 2021

View reviewed changes

remove else

019a057

raubitsj reviewed Nov 10, 2021

View reviewed changes

raubitsj approved these changes Nov 10, 2021

View reviewed changes

Merge branch 'master' into api-key-hotfix

28e2e93

dannygoldstein merged commit 338f223 into master Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WB-6865] ensure captive key input even if user presses enter #2721

[WB-6865] ensure captive key input even if user presses enter #2721

dannygoldstein commented Oct 3, 2021

codecov bot commented Oct 3, 2021 •

edited

Loading

raubitsj commented Oct 11, 2021 •

edited

Loading

dannygoldstein commented Oct 21, 2021

dannygoldstein commented Oct 21, 2021

vwrj commented Nov 4, 2021

dannygoldstein commented Nov 7, 2021

raubitsj Nov 10, 2021

dannygoldstein Nov 10, 2021

raubitsj Nov 10, 2021

dannygoldstein Nov 10, 2021

[WB-6865] ensure captive key input even if user presses enter #2721

[WB-6865] ensure captive key input even if user presses enter #2721

Conversation

dannygoldstein commented Oct 3, 2021

Description

Testing

Release Notes

codecov bot commented Oct 3, 2021 • edited Loading

Codecov Report

raubitsj commented Oct 11, 2021 • edited Loading

dannygoldstein commented Oct 21, 2021

dannygoldstein commented Oct 21, 2021

vwrj commented Nov 4, 2021

dannygoldstein commented Nov 7, 2021

raubitsj Nov 10, 2021

Choose a reason for hiding this comment

dannygoldstein Nov 10, 2021

Choose a reason for hiding this comment

raubitsj Nov 10, 2021

Choose a reason for hiding this comment

dannygoldstein Nov 10, 2021

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2021 •

edited

Loading

raubitsj commented Oct 11, 2021 •

edited

Loading