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

feat: add Client.logout and context manager #381

Merged
merged 7 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `wait_completion` param on `get_performances`, `list_task_output_assets` and `get_task_output_asset` to block execution until execution is over ([#368](https://github.com/Substra/substra/pull/368))
- `list_task_output_assets` and `get_task_output_asset` wait that the compute task is over before getting assets ([#369](https://github.com/Substra/substra/pull/369))
- warning and help message when logging in with username/password rather than token ([#378](https://github.com/Substra/substra/pull/378))
- new `Client.logout` function, mirroring `Client.login` ([#381](https://github.com/Substra/substra/pull/381))
- `Client` can now be used within a context manager ([#381](https://github.com/Substra/substra/pull/381))
```python
with Client(
client_name="org-1",
backend_type="remote",
url="http://substra-backend.org-1.com:8000",
username="org-1",
password="p@sswr0d44",
) as client:
pass
```

### Changed

- change how API responses are parsed to match server changes ([#379](https://github.com/Substra/substra/pull/379))
- `Client` will now terminate the sessions it starts when given username & password ([#381](https://github.com/Substra/substra/pull/381))

### Fixed

Expand Down
4 changes: 4 additions & 0 deletions substra/sdk/backends/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ def backend_mode(self) -> BackendType:
def login(self, username, password):
pass

@abc.abstractmethod
def logout(self):
pass

@abc.abstractmethod
def get(self, asset_type, key):
raise NotImplementedError
Expand Down
3 changes: 3 additions & 0 deletions substra/sdk/backends/local/backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ def backend_mode(self) -> schemas.BackendType:
def login(self, username, password):
self._db.login(username, password)

def logout(self):
self._db.logout()

def get(self, asset_type, key):
return self._db.get(asset_type, key)

Expand Down
4 changes: 4 additions & 0 deletions substra/sdk/backends/local/dal.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ def login(self, username, password):
if self._remote:
self._remote.login(username, password)

def logout(self):
if self._remote:
self._remote.logout()

def add(self, asset):
return self._db.add(asset)

Expand Down
3 changes: 3 additions & 0 deletions substra/sdk/backends/remote/backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ def backend_mode(self) -> schemas.BackendType:
def login(self, username, password):
return self._client.login(username, password)

def logout(self):
return self._client.logout()

def get(self, asset_type, key):
"""Get an asset by key."""
asset = self._client.get(asset_type.to_server(), key)
Expand Down
19 changes: 19 additions & 0 deletions substra/sdk/backends/remote/rest_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def __init__(self, url, insecure, token):
if not url:
raise exceptions.SDKException("url required to connect to the Substra server")
self._base_url = url[:-1] if url.endswith("/") else url
self._token_id = None # filled in by self.login

def login(self, username, password):
# we do not use self._headers in order to avoid existing tokens to be sent alongside the
Expand Down Expand Up @@ -90,6 +91,7 @@ def login(self, username, password):
try:
rj = r.json()
token = rj["token"]
self._token_id = rj["id"]
except json.decoder.JSONDecodeError:
# sometimes requests seem to be fine, but the json is not being found
# this might be if the url seems to be correct (in the syntax)
Expand All @@ -108,6 +110,23 @@ def login(self, username, password):

return token

def logout(self) -> None:
if self._token_id is None:
return
try:
r = requests.delete(
f"{self._base_url}/active-api-tokens/", params={"id": self._token_id}, headers=self._headers
)
r.raise_for_status()
self._token_id = None
logger.info("Successfully logged out")
except requests.exceptions.ConnectionError as e:
raise exceptions.ConnectionError.from_request_exception(e)
except requests.exceptions.Timeout as e:
raise exceptions.Timeout.from_request_exception(e)
Comment on lines +125 to +126
Copy link
Contributor

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 🤔

except requests.exceptions.HTTPError as e:
raise exceptions.HTTPError.from_request_exception(e)

# TODO: '__request' is too complex, consider refactoring
def __request(self, request_name, url, **request_kwargs): # noqa: C901
"""Base request helper."""
Expand Down
26 changes: 26 additions & 0 deletions substra/sdk/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure whether context manager is still needed with __del__ defined, but people online say not to rely on it and using with is preferable. However that does mean updating our documentation.

Copy link
Member

Choose a reason for hiding this comment

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

What part of the documentation are you reffering to ?

Copy link
Contributor Author

@oleobal oleobal Aug 16, 2023

Choose a reason for hiding this comment

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

Copy link
Member

@ThibaultFy ThibaultFy Aug 16, 2023

Choose a reason for hiding this comment

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

Some scripts (such as the examples) need the client for more than a thousand lines, so I would not replace the old version, maybe just indicate that the context manager is also a valid option ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it in the sdk.Client reference, under the username argument.


def _get_backend(self, backend_type: schemas.BackendType):
# Three possibilities:
# - remote: get a remote backend
Expand Down Expand Up @@ -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):
Expand Down
31 changes: 24 additions & 7 deletions tests/sdk/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,31 @@ def test_client_should_raise_when_missing_name():

def test_client_with_password(mocker):
mocker.patch("substra.sdk.Client.login", side_effect=stub_login)
client = Client(
backend_type="remote",
url="example.com",
token=None,
username="org-1",
password="password1",
)
rest_client_logout = mocker.patch("substra.sdk.backends.remote.rest_client.Client.logout")
client_args = {
"backend_type": "remote",
"url": "example.com",
"token": None,
"username": "org-1",
"password": "password1",
}

client = Client(**client_args)
assert client._token == "token1"
client.logout()
assert client._token is None
rest_client_logout.assert_called_once()
del client

rest_client_logout.reset_mock()
client = Client(**client_args)
del client
rest_client_logout.assert_called_once()

rest_client_logout.reset_mock()
with Client(**client_args) as client:
assert client._token == "token1"
rest_client_logout.assert_called_once()


def test_client_token_supercedes_password(mocker):
Expand Down
Loading