-
Notifications
You must be signed in to change notification settings - Fork 40
Improve get jobs and get job logic with new search parameters #1695
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
base: main
Are you sure you want to change the base?
Conversation
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.
Good job! this is nice.
Since I was working in the files migration in parallel, naturally we didn't developed same solution. I have some lines to discuss.
If you have some time, take a look to the PR and review it because you probably have some comments for me :)
gateway/api/repositories/jobs.py
Outdated
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.
clean and neat
gateway/api/use_cases/get_jobs.py
Outdated
def __init__( # pylint: disable=too-many-positional-arguments | ||
self, | ||
user, | ||
limit: Optional[int], | ||
offset: int = 0, | ||
type_filter: Optional[TypeFilter] = None, | ||
status: Optional[str] = None, | ||
created_after: Optional[datetime] = None, | ||
function_name: Optional[str] = None, | ||
): | ||
self.user = user | ||
self.limit = limit | ||
self.offset = offset | ||
self.type_filter = type_filter | ||
self.status = status | ||
self.created_after = created_after | ||
self.function_name = function_name |
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.
I have two comments here:
- To remove the "too many positional arguments" maybe you can use the
JobRepository
>JobFilter
dataclass
. I think it will also be more readable. - I was migrating the
files
endpoint and I put the args in the execute instead of the init. I think it is good to have consensus on this. In my opinion, most of this parameters are not a "permanent" state, are more related with the action. Also it simplifies the code, we don't have to set object variables, just use them.
What do you think?
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.
you are right, I though about passing JobFilters to the use case and discard it but it will be a better solution than passing all the individual filters.
I also like the idea of moving the arguments to the execute method.
class ProviderNotFoundException(Exception): | ||
"""Provider not found or access denied.""" | ||
|
||
|
||
class FunctionNotFoundException(Exception): | ||
"""Function not found or access denied.""" | ||
|
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.
In the files migration i created a generic NotFoundException
in the domain folder: https://github.com/Qiskit/qiskit-serverless/pull/1696/files#diff-e6da6866795b4c157d9b9e887eb27622f036a9dc512d30e5097a301df1d5001c
Also I created an additional decorator to quickly manage the possible NotFoundException
s (and other that can be created later) and create the corresponding Response
. The only cons is that you have to set the message when you create the exception to inform about the issue.
try: | ||
jobs, total = GetProviderJobsUseCase(**input_data).execute() | ||
except ProviderNotFoundException: | ||
return Response( | ||
{"message": f"Provider {input_data['provider']} doesn't exist."}, | ||
status=status.HTTP_404_NOT_FOUND, | ||
) | ||
except FunctionNotFoundException: | ||
return Response( | ||
{ | ||
"message": f"Qiskit Function {input_data['provider']}/{input_data['function_name']} doesn't exist." # pylint: disable=line-too-long | ||
}, | ||
status=status.HTTP_404_NOT_FOUND, | ||
) |
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.
In the files migration i created a generic NotFoundException
in the domain folder: https://github.com/Qiskit/qiskit-serverless/pull/1696/files#diff-e6da6866795b4c157d9b9e887eb27622f036a9dc512d30e5097a301df1d5001c
Also I created an additional decorator to quickly manage the possible NotFoundException
s (and other that can be created later) and create the corresponding Response
. The only cons is that you have to set the message when you create the exception to inform about the issue.
gateway/api/v1/views/get_jobs.py
Outdated
def serialize_input(request): | ||
"""Parse and validate query parameters from the request.""" | ||
user = request.user | ||
|
||
limit = parse_positive_int( | ||
request.query_params.get("limit"), settings.REST_FRAMEWORK["PAGE_SIZE"] | ||
) | ||
offset = parse_positive_int(request.query_params.get("offset"), 0) | ||
|
||
type_filter = None | ||
type_param = request.query_params.get("filter") | ||
if type_param: | ||
try: | ||
type_filter = TypeFilter(type_param) | ||
except ValueError as exc: | ||
raise ValueError( | ||
f"Invalid type filter. Must be one of: {', '.join([e.value for e in TypeFilter])}" | ||
) from exc | ||
|
||
status_filter = request.query_params.get("status") | ||
|
||
created_after = None | ||
created_after_param = request.query_params.get("created_after") | ||
if created_after_param: | ||
created_after = parse_datetime(created_after_param) | ||
if created_after is None: | ||
raise ValueError( | ||
"Invalid created_after format. Use ISO 8601 format (e.g., '2024-01-01T00:00:00Z')" | ||
) | ||
|
||
function_name = request.query_params.get("function") | ||
|
||
return { | ||
"user": user, | ||
"limit": limit, | ||
"offset": offset, | ||
"type_filter": type_filter, | ||
"status": status_filter, | ||
"created_after": created_after, | ||
"function_name": function_name, | ||
} |
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.
I think this is a bit complex to read. Maybe a Serializer
class fits better in here.
def serialize_input(request): | ||
""" | ||
Prepare the input for the end-point with validation | ||
""" | ||
user = request.user | ||
|
||
limit = parse_positive_int( | ||
request.query_params.get("limit"), settings.REST_FRAMEWORK["PAGE_SIZE"] | ||
) | ||
offset = parse_positive_int(request.query_params.get("offset"), 0) | ||
|
||
status_filter = request.query_params.get("status") | ||
|
||
created_after = None | ||
created_after_param = request.query_params.get("created_after") | ||
if created_after_param: | ||
created_after = parse_datetime(created_after_param) | ||
if created_after is None: | ||
raise ValueError( | ||
"Invalid created_after format. Use ISO 8601 format (e.g., '2024-01-01T00:00:00Z')" | ||
) | ||
|
||
provider = request.query_params.get("provider") | ||
function_name = request.query_params.get("function") | ||
if not provider or not function_name: | ||
raise ValueError("Qiskit Function title and Provider name are mandatory") | ||
|
||
return { | ||
"user": user, | ||
"limit": limit, | ||
"offset": offset, | ||
"status": status_filter, | ||
"created_after": created_after, | ||
"provider": provider, | ||
"function_name": function_name, | ||
} |
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.
Same here, maybe we can try with a Serlializer
class
gateway/api/v1/views/utils.py
Outdated
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.
cool
gateway/api/v1/urls.py
Outdated
@@ -46,4 +46,4 @@ | |||
basename=v1_views.CatalogViewSet.BASE_NAME, | |||
) | |||
|
|||
urlpatterns = router.urls + RouteRegistry.get() | |||
urlpatterns = RouteRegistry.get() + router.urls |
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.
nice catch
# Conflicts: # gateway/api/v1/views/files.py
the |
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.
I just left a couple of comments plus we need to add also in the client
the same filters as we added for the jobs
method.
) -> Optional[Function]: | ||
""" | ||
This method returns the specified function if the user is | ||
the author of the function or it has a permission. |
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.
I think we need to update the description as this method doesn't check the permission.
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.
Also the arguments need to be updated.
self.client.force_authenticate(user=user) | ||
provider = "default" | ||
function = "Docker-Image-Program" | ||
created_after = "2023-02-02T00:00:00.000000Z" |
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.
If I can be honest I don't think the user will introduce us this format in the date. Can we change the tests with created_after
with a more realistic date? Like: 2023-02-02
, for example.
limit = kwargs.get("limit", 10) | ||
kwargs["limit"] = limit | ||
offset = kwargs.get("offset", 0) | ||
kwargs["offset"] = offset | ||
status = kwargs.get("status", None) | ||
kwargs["status"] = status | ||
created_after = kwargs.get("created_after", None) |
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.
I would probably validate here also that the created_after
is a valid date with fromisoformat.
Summary
This PR adds the new jobs and provider_jobs parameters in the client:
And in the backend the changes applied are:
/jobs
and/jobs/provider
endpoints to new architecturestatus
,function
andcreated_after
dateProgramSummarySerializer
so we return less info about the program in the job listsDetails and comments
After trying multiple solutions, I think the best pattern is to return a QuerySet in the repository but not propagate it to the endpoint. The use-case should return a business object, not the QuerySet.
I created the
create_paginated_response
utility method that should be used in future endpoints to maintain consistency between responses.Also, the previous response was returning different response types. I removed the non-paginated response because it wasn't being used in the client.