Job registry #54

kpsherva · 2024-08-23T15:55:19Z

closes jobs registry #49

jrcastro2

Seems that it's still in progress, but looks like pretty good to me 💯

Left some questions to get some more context.

invenio_jobs/administration/jobs.py

invenio_jobs/administration/runs.py

invenio_jobs/assets/semantic-ui/js/invenio_jobs/administration/JobRunsHeader.js

invenio_jobs/assets/semantic-ui/js/invenio_jobs/administration/RunActionForm.js

invenio_jobs/models.py

invenio_jobs/services/services.py

kpsherva · 2024-08-26T14:45:45Z

invenio_jobs/models.py

    id = db.Column(UUIDType, primary_key=True, default=uuid.uuid4)
    active = db.Column(db.Boolean, default=True, nullable=False)
    title = db.Column(db.String(255), nullable=False)
    description = db.Column(db.Text)
-
+    # default_args = db.Column(JSON, default=lambda: dict(), nullable=True)


attention: this is a breaking change, please let me know if you want to discuss

invenio_jobs/alembic/1f896f6990b8_update_jobs_module_table_names.py

carlinmack · 2024-08-27T08:49:17Z

Looks good! Couple things

I think this arrow is pointing the wrong way? Normally it goes from > to v (like on https://about.zenodo.org/roadmap/) but it goes from v to <
I think the banner should go below the config, especially as you above in the text
~~It would be nice to have a "copy to clipboard" button~~ Ah I didn't realise that the reference config already has the ability to copy to clipboard! I think it only really makes sense to copy the full thing, but this implementation is fine as it is

I think the text should say "Modifying the Custom args field will replace any arguments specified above and run the task with the custom configuration." (italics for the added words), or just "Modifying the Custom args field will replace any arguments specified above"

carlinmack · 2024-08-27T11:56:59Z

My other feedback was about moving the Task selection to the top of the create page and auto-filling the task title

slint

LGTM overall, I think it's best as we said to discuss IRL at some point to go quickly through the comments

invenio_jobs/models.py

slint · 2024-08-29T13:14:31Z

invenio_jobs/services/schema.py

@@ -125,20 +159,28 @@ class Meta:

    task = fields.String(
        required=True,
-        validate=LazyOneOf(choices=lambda: current_jobs.tasks.keys()),
+        validate=LazyOneOf(choices=lambda: [name for name, t in Task.all().items()]),


Suggested change

validate=LazyOneOf(choices=lambda: [name for name, t in Task.all().items()]),

validate=LazyOneOf(choices=lambda: Task.all().keys(),

nit: isn't that the same?

invenio_jobs/services/services.py

slint · 2024-08-29T13:36:51Z

invenio_jobs/tasks.py

@@ -46,6 +46,7 @@ def execute_run(self, run_id, kwargs=None):
            run,
            status=RunStatusEnum.FAILED,
            finished_at=datetime.now(timezone.utc),
+            message=f"{e.__class__.__name__}: {str(e)}",


minor: maybe it makes sense to store the whole exception stacktrace here? I saw roughly how it looks in the UI in the demo, but e.g. if it's something small like KeyError: "id" not found, it'll be difficult to debug. Of course logging is the long term solution, but maybe for now that would be a small improvement that's good enough to get by

I will check if it is an easy win. Otherwise, I will ticketize it since it is a bit out of scope

slint · 2024-08-29T14:04:26Z

invenio_jobs/services/schema.py

+        load_default=lambda: current_jobs.default_queue,
+        dump_default=lambda: current_jobs.default_queue,


minor: this should default to the Job's default queue, i.e. job.default_queue.

invenio_jobs/models.py

slint · 2024-08-29T14:50:25Z

invenio_jobs/models.py

    schedule = db.Column(JSON, nullable=True)

    @property
    def last_run(self):
        """Last run of the job."""
-        return self.runs.order_by(Run.created.desc()).first()
+        _run = self.runs.order_by(Run.created.desc()).first()
+        return _run if _run else {}


minor: isn't it cleaner/more-consistent to return None here?

slint · 2024-08-29T15:02:44Z

invenio_jobs/services/schema.py

+
+    def __init__(self, *args, **kwargs):
+        """Constructor."""
+        self.type_schemas = deepcopy(current_jobs.registry.registered_schemas())


minor: can this be "lazy", via e.g. a property? In general when we start using current_xxx and Flask app-context in __init__, we end up in difficult situations in config/schemas.

invenio_jobs/assets/semantic-ui/js/invenio_jobs/administration/ScheduleJobModal.js

zzacharo · 2024-09-04T15:56:51Z

invenio_jobs/services/schema.py

+        lambda: RegisteredTaskArgumentsSchema,
+        metadata={
+            "type": "dynamic",
+            "endpoint": "/api/tasks/<item_id>/args",


question: out of curiosity how is this metadata used?

zzacharo · 2024-09-04T15:57:08Z

invenio_jobs/services/schema.py

+        metadata={
+            "type": "dynamic",
+            "endpoint": "/api/tasks/<item_id>/args",
+            "depends_on": "task",


question: also this?

tests/resources/test_resources.py

invenio_jobs/jobs.py

invenio_jobs/registry.py

ntarocco · 2024-09-09T12:24:00Z

invenio_jobs/assets/semantic-ui/js/invenio_jobs/administration/JobActions.js

    this.setState({
      modalOpen: false,
      modalHeader: undefined,
      modalBody: undefined,
    });
    setTimeout(() => {
-      window.location.reload();
+      window.location = resource.links.self_admin_html;


What is this timeout of 1 sec?

This is when we create a new job run and we switch to detail view of this run (to monitor it).
@0einstein0 was the 1s chosen for any specific reason?

invenio_jobs/assets/semantic-ui/js/invenio_jobs/administration/RunActionForm.js

zzacharo · 2024-09-11T12:44:15Z

invenio_jobs/registry.py

@@ -34,11 +34,12 @@ def get_job_id(self, instance):
                return job_id
        raise KeyError("Job not found in registry.")

-    def all_registered_jobs(self):
+    def get_all(self):


similary this could be a property as well i.e. all methods of def method(self)

carlinmack · 2024-09-12T09:59:29Z

Do the screenshots in the first comment need updating?

* tests: adapt tests to new implementation of jobs registry * chore: fix formatting

* tests: fix expected outputs

jrcastro2 reviewed Aug 26, 2024

View reviewed changes

kpsherva force-pushed the job-registry branch 5 times, most recently from af84dd6 to 9b78b9d Compare August 26, 2024 14:44

kpsherva commented Aug 26, 2024

View reviewed changes

invenio_jobs/alembic/1f896f6990b8_update_jobs_module_table_names.py Show resolved Hide resolved

kpsherva force-pushed the job-registry branch 2 times, most recently from 5eb1349 to cffab7f Compare August 28, 2024 13:14

slint reviewed Aug 29, 2024

View reviewed changes

zzacharo reviewed Sep 4, 2024

View reviewed changes

ntarocco reviewed Sep 9, 2024

View reviewed changes

zzacharo reviewed Sep 11, 2024

View reviewed changes

kpsherva added 11 commits September 17, 2024 15:07

tasks: add registry of tasks

94f0ff0

administration: customize jobs views

b445620

alembic: change jobs table names for consistency

0781a85

* tests: adapt tests to new implementation of jobs registry * chore: fix formatting

assets: run action form UX improvement

df40fef

run: serialize arguments for celery task

b7064fe

assets: add translations

014ea45

jobs: rename registred class

23102bd

chore: add files to manifest

04d9c41

translations: add translation json to assets

4eb4d85

admin form: add default job queue

d03407c

* tests: fix expected outputs

run: handle run on the details page

7ffc045

kpsherva force-pushed the job-registry branch 2 times, most recently from 359ccf7 to e50f4a9 Compare September 17, 2024 14:13

chore: formatting

77f2c45

kpsherva force-pushed the job-registry branch from e50f4a9 to 77f2c45 Compare September 25, 2024 15:13

kpsherva merged commit 36e59c7 into inveniosoftware:master Sep 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Job registry #54

Job registry #54

kpsherva commented Aug 23, 2024 •

edited

Loading

jrcastro2 left a comment

kpsherva Aug 26, 2024

carlinmack commented Aug 27, 2024 •

edited

Loading

carlinmack commented Aug 27, 2024

slint left a comment

slint Aug 29, 2024

slint Aug 29, 2024

kpsherva Sep 9, 2024

slint Aug 29, 2024

slint Aug 29, 2024

slint Aug 29, 2024

zzacharo Sep 4, 2024

zzacharo Sep 4, 2024

ntarocco Sep 9, 2024

kpsherva Sep 9, 2024

zzacharo Sep 11, 2024

carlinmack commented Sep 12, 2024

	validate=LazyOneOf(choices=lambda: [name for name, t in Task.all().items()]),
	validate=LazyOneOf(choices=lambda: Task.all().keys(),

		load_default=lambda: current_jobs.default_queue,
		dump_default=lambda: current_jobs.default_queue,

Job registry #54

Job registry #54

Conversation

kpsherva commented Aug 23, 2024 • edited Loading

jrcastro2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlinmack commented Aug 27, 2024 • edited Loading

carlinmack commented Aug 27, 2024

slint left a 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

carlinmack commented Sep 12, 2024

kpsherva commented Aug 23, 2024 •

edited

Loading

carlinmack commented Aug 27, 2024 •

edited

Loading