added more query params #98

marjo-luc · 2024-07-31T01:35:35Z

Added support for the following optional query params supported by the DPS for the listJobs endpoint:
algo_id, end_time, get_job_details, offset, page_size, queue, start_time, status, tag, version

Sample maap-py request:
maap.listJobs("mlucas", algo_id="job-dps_tutorial_anil_test_v2", version="main", queue="maap-dps-worker-8gb", page_size=1, get_job_details=False)

The get_job_details flag is set to True by default and if set to False will return only the job id's and associated job tags.

maap/maap.py

chuckwondo · 2024-08-01T14:46:52Z

maap/maap.py

@@ -256,13 +256,22 @@ def cancelJob(self, jobid):
        job.id = jobid
        return job.cancel_job()

-    def listJobs(self, username=None, page_size=None, offset=None):
+    def listJobs(self, username=None, **kwargs):


I'm not sure we should take an arbitrary kwargs parameter. I would lean towards adding explicit keyword arguments, as keyword-only parameters, something like this (incomplete):

def listJobs(self, *, username=None, algo_id=None, version=None, ...): ...

Otherwise, the only way to know the available options is to look at the source code.

I might be swayed to keep what you have only if you add a complete docstring describing all of the supported options.

It makes no real difference to me. Any future updates to the param list will require a similar level of effort for code changes. I can make them explicit.

maap/maap.py

chuckwondo · 2024-08-01T14:56:53Z

maap/maap.py

        if username==None and self.profile is not None and 'username' in self.profile.account_info().keys():
            username = self.profile.account_info()['username']

        url = os.path.join(self.config.dps_job, username, endpoints.DPS_JOB_LIST)
-        params = {k: v for k, v in (("page_size", page_size), ("offset", offset)) if v}
+        valid_keys = ['algo_id', 'end_time', 'get_job_details', 'offset', 'page_size', 'priority', 'queue', 'start_time', 'status', 'tag', 'version']


How do these affect behavior?

start_time and end_time: what's the type and/or format, and does the API do an exact match on these?

get_job_details: is this a bool? if so, what does it do, simply return more details for each returned job?

priority: what is this? a number? what does it represent?

Regardless of whether you apply my suggestion to define explicit keyword parameters, please add a docstring that describes the data types and what they are for (particularly for the non-obvious ones I mentioned above)

maap/maap.py

chuckwondo · 2024-08-01T15:15:17Z

maap/maap.py

-        params = {k: v for k, v in (("page_size", page_size), ("offset", offset)) if v}
+        valid_keys = ['algo_id', 'end_time', 'get_job_details', 'offset', 'page_size', 'priority', 'queue', 'start_time', 'status', 'tag', 'version']
+
+        params = {k: v for k, v in kwargs.items() if k in valid_keys and v is not None}


Suggested change

params = {k: v for k, v in kwargs.items() if k in valid_keys and v is not None}

params = {

k: v

for k, v in (

("username", username),

("algo_id", algo_id),

...

)

if v is not None

}

Co-authored-by: Chuck Daniels <[email protected]>

chuckwondo · 2024-08-01T22:14:21Z

maap/maap.py

@@ -256,13 +256,76 @@ def cancelJob(self, jobid):
        job.id = jobid
        return job.cancel_job()

-    def listJobs(self, username=None, page_size=None, offset=None):
-        if username==None and self.profile is not None and 'username' in self.profile.account_info().keys():
+    def listJobs(self, username=None, 


Given the large number of options, use keyword-only parameters. Specifically, inserting the * forces the caller to use keywords for any parameters listed after the *.

Suggested change

def listJobs(self, username=None,

def listJobs(self, *, username=None,

This means you must call the function like so: listJobs(username=username, algo_id="myalgo", ...)

Otherwise, it can be very confusing with so many args if you don't force use of keywords.

This would be a breaking change in the interface, which I do not want to do without first communicating the change to users. I made all parameters other than username keyword-only params.

More broadly speaking, I believe we have plans to cleanup maap-py in the nearish future and can revisit this then.

chuckwondo · 2024-08-01T22:15:36Z

maap/maap.py

+            username (str): Platform user.
+            algo_id (str): Algorithm type.


Are these optional?

Yes. Turns out username is technically optional.

chuckwondo · 2024-08-01T22:18:43Z

maap/maap.py

+            start_time (str, optional): Specifying this parameter will return all jobs that have started from the provided start time to now. e.g. 2024-01-01 or 2024-01-01T00:00:00.000000Z.
+            status (str, optional): Job status e.g. job-completed, job-failed, job-started, job-queued.
+            tag (str, optional): User job tag.
+            version (str, optional): Algorithm version i.e. GitHub branch.


Suggested change

version (str, optional): Algorithm version i.e. GitHub branch.

version (str, optional): Algorithm version (e.g., a GitHub branch name or a tag)

chuckwondo · 2024-08-01T22:19:30Z

maap/maap.py

+            queue (str, optional): Job processing resource.
+            start_time (str, optional): Specifying this parameter will return all jobs that have started from the provided start time to now. e.g. 2024-01-01 or 2024-01-01T00:00:00.000000Z.
+            status (str, optional): Job status e.g. job-completed, job-failed, job-started, job-queued.
+            tag (str, optional): User job tag.


Suggested change

tag (str, optional): User job tag.

tag (str, optional): User job tag/identifier

sujen1412 · 2024-08-02T00:09:01Z

maap/maap.py

+            end_time (str, optional): Specifying this parameter will return all jobs that have completed from the provided end time to now. e.g. 2024-01-01 or 2024-01-01T00:00:00.000000Z.
+            get_job_details (bool, optional): Flag that determines whether to return a detailed job list or a compact list containing just the job ids and their associated job tags. Default is True.
+            offset (int, optional): Offset for pagination.
+            page_size (int, optional): Page size for pagination.


What is the default page_size ? I think we should have the default number in the function definition instead of None

Same goes for offset, should be set to 0 instead of None

I've updated the defaults to be consistent with what we are using in the MAAP API

offset: 0
page_size: 10

sujen1412 · 2024-08-02T00:10:33Z

maap/maap.py

+            get_job_details (bool, optional): Flag that determines whether to return a detailed job list or a compact list containing just the job ids and their associated job tags. Default is True.
+            offset (int, optional): Offset for pagination.
+            page_size (int, optional): Page size for pagination.
+            priority (int, optional): Job processing priority. Valid values are integers from 0-9.


Currently, we do not use priorities on MAAP, would providing a value (0-9) accidentally prevent users from seeing their jobs ? If yes, then we should not expose this parameter to maap-py.

Removed priority param.

sujen1412 · 2024-08-02T00:14:04Z

maap/maap.py

+        Returns:
+            list: List of jobs for a given user that matches query params provided.
+        """
+        if username is None and self.profile is not None and 'username' in self.profile.account_info().keys():


If the code is not able to determine a username, should it continue or throw an error ?

Added more error handling to handle this.

sujen1412 · 2024-08-02T00:19:24Z

maap/maap.py

+        Returns:
+            list: List of jobs for a given user that matches query params provided.
+        """
+        if username is None and self.profile is not None and 'username' in self.profile.account_info().keys():


could we have a function to determine username ?

Suggested change

if username is None and self.profile is not None and 'username' in self.profile.account_info().keys():

username = username if username else self.get_username()

if username is None:

raise SomeException

We could probably clean this up a bit. I'm opting to looking at this when we run through a cleanup of maap-py.

sujen1412 · 2024-08-02T15:35:23Z

maap/maap.py

+            priority (int, optional): Job processing priority. Valid values are integers from 0-9.
+            queue (str, optional): Job processing resource.
+            start_time (str, optional): Specifying this parameter will return all jobs that have started from the provided start time to now. e.g. 2024-01-01 or 2024-01-01T00:00:00.000000Z.
+            status (str, optional): Job status e.g. job-completed, job-failed, job-started, job-queued.


The job status field in maappy does not map exactly the same with HySDS.
So we do not have job-completed, job-failed, job-started, job-queued instead we have Accepted, Running, Succeeded, Failed, Dismissed, Deleted, Deduped and Offline. Have a look here - https://github.com/MAAP-Project/maap-api-nasa/blob/main/api/utils/ogc_translate.py#L22-L54

sujen1412 · 2024-08-06T16:19:15Z

maap/maap.py


        Returns:
            list: List of jobs for a given user that matches query params provided.
        """
        if username is None and self.profile is not None and 'username' in self.profile.account_info().keys():
            username = self.profile.account_info()['username']

+        if username is None:
+            raise ValueError("Username must be supplied.")


Since the username field is optional, I think this error should be more descriptive.
Something like, Unable to determine username from profile, please provide a username

Thoughts:
In the future we would need to add a method to explicitly do maap.login() by providing the PGT token if not found in the env vars, and then update this error message to be Unable to determine username from profile, please ensured you are logged in by calling maap.login() cc @bsatoriu

maap/utils/JobUtils.py

sujen1412 · 2024-08-06T16:29:44Z

maap/maap.py

@@ -256,13 +257,12 @@ def cancelJob(self, jobid):
        job.id = jobid
        return job.cancel_job()

-    def listJobs(self, username=None, 
+    def listJobs(self, username=None, *,


What is this * for ?

The * means that any parameters that appear after it in the parameter list must be supplied using keywords. In this case, username is the only parameter that can be supplied without a keyword, but everything else must be supplied with keywords. This is very helpful when there are many possible options, but you don't want to have to pass every argument in the correct order, supplying None value for args you don't care about.

maap/utils/JobUtils.py

marjo-luc · 2024-08-14T01:31:12Z

@chuckwondo do you have any final comments?

chuckwondo

Minor comment about renaming job_utils.py to jobs.py, but approving.

chuckwondo · 2024-09-03T22:02:59Z

maap/maap.py

@@ -19,6 +19,7 @@
 from maap.AWS import AWS
 from maap.dps.DpsHelper import DpsHelper
 from maap.utils import endpoints
+from maap.utils import job_utils


minor: can we just name it jobs since already in the utils package? this would also make it consistent with endpoints

chuckwondo · 2024-09-04T00:00:09Z

maap/maap.py

@@ -19,6 +19,7 @@
 from maap.AWS import AWS
 from maap.dps.DpsHelper import DpsHelper
 from maap.utils import endpoints
+from maap.utils import job


jobs (plural), but perhaps during the "cleanup" you mentioned

* Remove the need to track maap.cfg file (#93) * Remove the need to track maap.cfg * Fix merge issues * Allow overriding of MAAP_API_HOST * Split config url construction into functions * Add test for config reader * Add script to perform functional test of maap-py * Construct api root url from provided maap host * Apply suggestions from code review Co-authored-by: Chuck Daniels <[email protected]> * Apply suggestions from code review to update url handling Co-authored-by: Chuck Daniels <[email protected]> * Remove unused code and memoize get config function * Update request utils to drop class and use methods * Use new request utils and pass maap config object * Update imports and url handling * Simplify boolean self-signed for request utils * Remove usage of os.path.join for constructing urls * Fix variable name in exception message * Remove incorrect import * Update functional tests --------- Co-authored-by: Chuck Daniels <[email protected]> * added pagination to listJobs endpoint * change request * Issues/95: Add CICD pipeline (#101) * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Setup CICD * Fetch full repo in checkout for sonar analysis * tweak pr template wording * /version 4.0.1a0 * /version 4.1.0a1 * added more query params (#98) * added more query params * added get_job_details flag * cleanup * Update maap/maap.py Co-authored-by: Chuck Daniels <[email protected]> * Update maap/maap.py Co-authored-by: Chuck Daniels <[email protected]> * Update maap/maap.py Co-authored-by: Chuck Daniels <[email protected]> * added docstring | updated param handling * review updates * review updates * renamed file --------- Co-authored-by: Chuck Daniels <[email protected]> * /version 4.1.0a2 * secret management (#104) * wip * cleanup * updated get_secret endpoint to return value * added tests for secrets management * updated error handling | added logging * review comments * /version 4.1.0a3 * /version 4.2.0a0 * added changelog entry * /version 4.1.0rc1 --------- Co-authored-by: Sujen Shah <[email protected]> Co-authored-by: Chuck Daniels <[email protected]> Co-authored-by: Frank Greguska <[email protected]> Co-authored-by: frankinspace <[email protected]> Co-authored-by: marjo-luc <[email protected]>

marjo-luc added 2 commits July 30, 2024 17:51

added more query params

b43ac1a

added get_job_details flag

f849719

marjo-luc self-assigned this Jul 31, 2024

cleanup

ba2fbad

marjo-luc marked this pull request as ready for review July 31, 2024 16:21

marjo-luc requested review from chuckwondo and sujen1412 July 31, 2024 16:21

marjo-luc mentioned this pull request Jul 31, 2024

Update MAAP API job list endpoint MAAP-Project/maap-api-nasa#130

Closed

3 tasks

chuckwondo requested changes Aug 1, 2024

View reviewed changes

marjo-luc and others added 4 commits August 1, 2024 10:14

Update maap/maap.py

ae91831

Co-authored-by: Chuck Daniels <[email protected]>

Update maap/maap.py

1e2426a

Co-authored-by: Chuck Daniels <[email protected]>

Update maap/maap.py

1878389

Co-authored-by: Chuck Daniels <[email protected]>

added docstring | updated param handling

d7f3f05

marjo-luc requested a review from chuckwondo August 1, 2024 19:18

chuckwondo requested changes Aug 1, 2024

View reviewed changes

chuckwondo reviewed Aug 1, 2024

View reviewed changes

sujen1412 requested changes Aug 2, 2024

View reviewed changes

review updates

7e91c6d

marjo-luc requested review from chuckwondo and sujen1412 August 5, 2024 20:33

sujen1412 reviewed Aug 6, 2024

View reviewed changes

chuckwondo reviewed Aug 6, 2024

View reviewed changes

maap/utils/JobUtils.py Outdated Show resolved Hide resolved

review updates

343ee53

marjo-luc requested review from sujen1412 and chuckwondo August 6, 2024 17:45

sujen1412 previously approved these changes Aug 6, 2024

View reviewed changes

chuckwondo previously approved these changes Sep 3, 2024

View reviewed changes

renamed file

5b12d72

marjo-luc dismissed stale reviews from chuckwondo and sujen1412 via 5b12d72 September 3, 2024 23:55

marjo-luc requested a review from chuckwondo September 3, 2024 23:56

chuckwondo reviewed Sep 4, 2024

View reviewed changes

chuckwondo approved these changes Sep 4, 2024

View reviewed changes

marjo-luc merged commit 102869c into develop Sep 4, 2024

marjo-luc deleted the feature/jobs-list-query branch September 4, 2024 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added more query params #98

added more query params #98

marjo-luc commented Jul 31, 2024 •

edited

Loading

chuckwondo Aug 1, 2024

marjo-luc Aug 1, 2024

chuckwondo Aug 1, 2024

chuckwondo Aug 1, 2024

chuckwondo Aug 1, 2024

marjo-luc Aug 5, 2024

chuckwondo Aug 1, 2024

marjo-luc Aug 5, 2024

chuckwondo Aug 1, 2024

chuckwondo Aug 1, 2024

sujen1412 Aug 2, 2024

marjo-luc Aug 5, 2024

sujen1412 Aug 2, 2024

marjo-luc Aug 5, 2024

sujen1412 Aug 2, 2024

marjo-luc Aug 5, 2024

sujen1412 Aug 2, 2024

marjo-luc Aug 5, 2024

sujen1412 Aug 2, 2024

sujen1412 Aug 6, 2024

sujen1412 Aug 6, 2024

chuckwondo Aug 6, 2024

marjo-luc commented Aug 14, 2024

chuckwondo left a comment

chuckwondo Sep 3, 2024

chuckwondo Sep 4, 2024

	def listJobs(self, username=None,
	def listJobs(self, *, username=None,

		username (str): Platform user.
		algo_id (str): Algorithm type.

	version (str, optional): Algorithm version i.e. GitHub branch.
	version (str, optional): Algorithm version (e.g., a GitHub branch name or a tag)

	tag (str, optional): User job tag.
	tag (str, optional): User job tag/identifier

added more query params #98

added more query params #98

Conversation

marjo-luc commented Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marjo-luc commented Aug 14, 2024

chuckwondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marjo-luc commented Jul 31, 2024 •

edited

Loading