Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Nov 10, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -981,3 +981,22 @@ def test_sync_wandb_run_and_tensorboard(runner, live_mock_server):
"WARNING Found .wandb file, not streaming tensorboard metrics"
in result.output
)


def test_cli_login_reprompts_when_no_key_specified(
mocker, runner, empty_netrc, local_netrc
):
with runner.isolated_filesystem():
mocker.patch("wandb.wandb_lib.apikey.getpass.getpass", input)
# this first gives login an empty API key, which should cause
# it to reprompt. this is what we are testing. we then give
# it a valid API key (the dummy API key with a different final
# letter to check that our monkeypatch input is working as
# expected) to terminate the prompt finally we grep for the
# Error: No API key specified to assert that the reprompt
# happened
result = runner.invoke(cli.login, input=f"\n{DUMMY_API_KEY[:-1]}q\n")
debug_result(result, "login")
with open("netrc", "r") as f:
print(f.read())
assert "ERROR No API key specified." in result.output
9 changes: 7 additions & 2 deletions wandb/sdk/lib/apikey.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ def prompt_api_key( # noqa: C901
choices, input_timeout=settings.login_timeout, jupyter=jupyter
)

api_ask = "%s: Paste an API key from your profile and hit enter: " % log_string
api_ask = (
"%s: Paste an API key from your profile and hit enter, or press ctrl+c to quit: "
% log_string
)
if result == LOGIN_CHOICE_ANON:
key = api.create_anonymous_api_key()

Expand Down Expand Up @@ -126,6 +129,8 @@ def prompt_api_key( # noqa: C901
elif result == LOGIN_CHOICE_NOTTY:
# TODO: Needs refactor as this needs to be handled by caller
return False
elif result == LOGIN_CHOICE_DRYRUN:
return None
else:
# Jupyter environments don't have a tty, but we can still try logging in using
# the browser callback if one is supplied.
Expand Down Expand Up @@ -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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


# TODO(jhr): api shouldn't be optional or it shouldnt be passed, clean up callers
api = api or InternalApi()
Expand Down
41 changes: 21 additions & 20 deletions wandb/sdk/wandb_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,26 +177,27 @@ def update_session(
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()

except TimeoutError:
wandb.termlog("W&B disabled due to login timeout.")
return None, ApiKeyStatus.DISABLED
if key is False:
return None, ApiKeyStatus.NOTTY
if not key:
return None, ApiKeyStatus.OFFLINE
return key, ApiKeyStatus.VALID
while True:
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])
continue
except TimeoutError:
wandb.termlog("W&B disabled due to login timeout.")
return None, ApiKeyStatus.DISABLED
if key is False:
return None, ApiKeyStatus.NOTTY
if not key:
return None, ApiKeyStatus.OFFLINE
else:
return key, ApiKeyStatus.VALID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed else


def prompt_api_key(self):
key, status = self._prompt_api_key()
Expand Down