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

Fix/SK-1127 | add session_id flag #742

Merged
merged 11 commits into from
Nov 20, 2024
Merged

Fix/SK-1127 | add session_id flag #742

merged 11 commits into from
Nov 20, 2024

Conversation

KatHellg
Copy link
Contributor

@KatHellg KatHellg commented Nov 8, 2024

added session_id flag in model_cmd.py, round_cmd.py, statuses_cmd.py, and validation_cmd.py. Works well but might be a brute force solution so please lmk if there's a smarter way of doing it.

also added an id flag in each file to be able to search based on id (e.g., model_id in model_cmd.py)

@KatHellg KatHellg changed the title session_id flag added Fix/SK-1127 | add session_id flag Nov 8, 2024
@KatHellg KatHellg requested review from niklastheman and Wrede and removed request for niklastheman November 11, 2024 12:24
Copy link
Contributor

@niklastheman niklastheman left a comment

Choose a reason for hiding this comment

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

Cool, I think I would like to have the "id functionality" (list_clients for example) refactored in separate functions (not part of list). The implementation of this looks good! The "session_id functionality" can be implemented in an other way. The GET [entity]/ endpoints accepts url params for filtering. For example: GET /rounds?session_id=<session_id_value>

Copy link
Contributor

@niklastheman niklastheman left a comment

Choose a reason for hiding this comment

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

Cool, good work! I have some comments, minor things...

fedn/cli/client_cmd.py Show resolved Hide resolved
fedn/cli/model_cmd.py Outdated Show resolved Hide resolved
fedn/cli/package_cmd.py Outdated Show resolved Hide resolved
fedn/cli/client_cmd.py Outdated Show resolved Hide resolved
@KatHellg KatHellg added HOLD and removed HOLD labels Nov 13, 2024


click.echo(f"\nRetrieving client: {url}\n")
click.echo(f"Headers: {headers}")
Copy link
Member

Choose a reason for hiding this comment

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

remove print of hearder

@@ -65,12 +65,46 @@ def list_clients(ctx, protocol: str, host: str, port: str, token: str = None, n_

try:
response = requests.get(url, headers=headers)
print_response(response, "clients")
print_response(response, "clients", None)
except requests.exceptions.ConnectionError:
Copy link
Member

Choose a reason for hiding this comment

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

We should handle more than just ConnectionError, there are plenty of other error related to http requests. You don't have do do this know but make an other issue of it. For example create a utility function that handles all requests and checks the response for error:

import requests
from requests.exceptions import HTTPError, Timeout, ConnectionError, RequestException

def make_request(method, url, **kwargs):
"""
A reusable function to handle HTTP(S) requests with error handling.

Args:
    method (str): HTTP method (e.g., 'GET', 'POST').
    url (str): The URL for the request.
    **kwargs: Additional arguments passed to `requests.request()`.

Returns:
    dict: A dictionary with the response data or an error message.
"""
try:
    response = requests.request(method, url, **kwargs)
    response.raise_for_status()
    # Assuming JSON response, adjust if needed
    return {"success": True, "data": response.json()}
except HTTPError as http_err:
    return {"success": False, "error": f"HTTP error occurred: {http_err}"}
except Timeout:
    return {"success": False, "error": "The request timed out"}
except ConnectionError:
    return {"success": False, "error": "Connection error occurred"}
except RequestException as req_err:
    return {"success": False, "error": f"Request error: {req_err}"}
except Exception as e:
    return {"success": False, "error": f"Unexpected error: {e}"}



click.echo(f"\nRetrieving combiner: {url}\n")
click.echo(f"Headers: {headers}")
Copy link
Member

Choose a reason for hiding this comment

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

remove

url = f"{url}{id}"

click.echo(f"\nRetrieving model: {url}\n")
click.echo(f"Headers: {headers}")
Copy link
Member

Choose a reason for hiding this comment

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

remove

"""Prints the api response to the cli.
:param response:
type: array
description: list of entities
:param entity_name:
type: string
description: name of entity
:param so:
type: boolean
desriptions: single output format (y/n)
Copy link
Member

Choose a reason for hiding this comment

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

remove (y/n)

click.echo(f"\t{k}: {v}")
click.echo("}")
else:
count, result = json_data.values()
Copy link
Member

Choose a reason for hiding this comment

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

we should consider using json pretty prints instead, however I would prefer not introducing a new dependency. Save for other issue

Copy link
Member

@Wrede Wrede left a comment

Choose a reason for hiding this comment

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

Good! Just remove all headers prints, I don't know why we have them, more as a debug print it feels. Had some comments that you can make new issues from. Finally, you should add unittests for cli cmds to make sure they work as expected.

@KatHellg KatHellg requested a review from Wrede November 18, 2024 09:19
@Wrede Wrede merged commit 63c56f9 into master Nov 20, 2024
19 checks passed
@Wrede Wrede deleted the fix/SK-1127 branch November 20, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants