-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: add Client.logout and context manager #381
Changes from all commits
c518351
e8c5ac7
01c3d37
f8d7f08
5a681b7
1370803
c809973
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -199,6 +199,12 @@ class Client: | |
Defaults to None. | ||
username (str, optional): Username to authenticate to the Substra platform. | ||
Used in conjunction with a password to generate a token if not given, using the `login` function. | ||
|
||
If using username/password, you should use a context manager to ensure the session terminates as intended: | ||
``` | ||
with Client(username, password) as client: | ||
... | ||
``` | ||
Not stored. | ||
Defaults to None. | ||
password (str, optional): Password to authenticate to the Substra platform. | ||
|
@@ -311,6 +317,15 @@ def __init__( | |
) | ||
self._token = self.login(config_dict["username"].value, config_dict["password"].value) | ||
|
||
def __enter__(self): | ||
return self | ||
|
||
def __exit__(self, exc_type, exc_value, traceback): | ||
self.logout() | ||
|
||
def __del__(self): | ||
self.logout() | ||
Comment on lines
+320
to
+327
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsure whether context manager is still needed with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What part of the documentation are you reffering to ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean that in the documentation all examples of the form: client = Client(...)
do_stuff(client) should be replaced with: with Client(...) as client:
do_stuff(client) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some scripts (such as the examples) need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it in the |
||
|
||
def _get_backend(self, backend_type: schemas.BackendType): | ||
# Three possibilities: | ||
# - remote: get a remote backend | ||
|
@@ -369,6 +384,17 @@ def login(self, username, password): | |
self._token = self._backend.login(username, password) | ||
return self._token | ||
|
||
@logit | ||
def logout(self) -> None: | ||
""" | ||
Log out from a remote server, if Client.login was used | ||
(otherwise, nothing happens) | ||
""" | ||
if not self._backend: | ||
raise exceptions.SDKException("No backend found") | ||
self._backend.logout() | ||
self._token = None | ||
|
||
@staticmethod | ||
def _get_spec(asset_type, data): | ||
if isinstance(data, asset_type): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to retry on Timeout? Or is it already handled at a higher level?
In all cases, I'm not sure if we want to raise uncaught exceptions to the user, or just have a warning logged 🤔