Add Blueprint and lazy registration pattern #423

tomdottom · 2021-07-26T18:45:36Z

Enables collections of tasks to be defined independently of the app
itself and registered at a later time.

Closes #421

Successful PR Checklist:

Tests
- (not applicable?)
Documentation
- (not applicable?)

tomdottom · 2021-07-26T19:08:57Z

@ewjoachim I wasn't sure how procrastinate deals with collisions of task names & task aliases.
This is probably more likely to happen with aliases than with fully qualified task names (fqtn).

I would have thought that adding the same task name or alias SHOULD raise an exception.

We could force a blueprint to have a namespace. Say:

bp = Blueprint(name="my-bp")

Or maybe leave this until the registration:

app.register_blueprint(ns="my-ns", blueprint=bp)

What do you think?

tomdottom · 2021-07-26T19:10:10Z

Also, do you want to call them Blueprints or is there a better name?
Maybe:

TaskCollections
Collections
Scaffold
TaskGroup
etc ...

ewjoachim · 2021-07-27T08:00:37Z

Or maybe leave this until the registration:
app.register_blueprint(ns="my-ns", blueprint=bp)

I like this idea because it makes it the responsibility of the caller (the only one who knows the different blueprints that will be combined) to ensure there is no overlap.

Also, do you want to call them Blueprints or is there a better name?

Blueprints seems clear enough to me. 👍

ewjoachim

Congratulations on a stellar work so far. Let's get he discussion going but I think we should be able to merge this soon !

docs/howto/blueprints.rst

procrastinate/blueprints.py

ewjoachim · 2021-07-27T11:04:33Z

procrastinate/blueprints.py

+    from procrastinate import app, tasks
+
+
+class BluePrint:


Hm... or could App inherit Blueprint ? That would solve the problem.

A nice advantage of that if it's possible is that register_blueprint could be called on an app but also on a blueprint, allowing recursive usage. Or allowing registering tasks from another app.

I'm not sure I see the problem.

The process involved in App.register_blueprint and Blueprint.register_blueprint would be different owing to the fact that the former actually binds tasks to the App, but the latter is creating structures to be bound at a later time.

Allowing a blueprint to be registered in a nested fashion would probably be as simple as:

class Blueprint: def __init__(self): self._blueprints = [] def register_blueprint(self, blueprint: "Blueprint") -> None: self._blueprints.append(blueprint)

The App.register_blueprint would then be responsible for traversing the tree of nested blueprints.

Is this something for this PR, or a separate one?

I'm not sure I see the problem.

The problem is ensuring that Blueprint.task() and App.task() signatures stay synchronized, but we've gone another route in the other thread.

Yep, if we don't do the recursive this now, I believe another PR will be fine. Also, we'll probably want to include a namespace here as well :D

tomdottom · 2021-07-27T11:25:49Z

@ewjoachim given that there's no concept of a task namespace in the db schema it will obviously have to be stored by convention in the task name & aliases.

From what I understand a job can be created by it's task name, and a worker can look up a jobs code using the task name or any of its aliases. So aliases primary use case is to allow task name changes and still allow workers to look up jobs based on old names.

With the ☝️ in mind it seems that only a task name should be prefixed with a namespace, and that it's the users responsibility to include any namespaces in the alias. Does this seem right?

As for the namespace prefix what type of delimiter would you prefer? :

ns.my.task
ns:my.task
ns::my.task
ns/my.task
etc ...

ewjoachim · 2021-07-27T14:05:30Z

As of today, within a worker, if a task is not registered, Procrastinate makes a last chance attempt and tries to load the Python object pointed at that dotted path. If it's a Procrastinate tasks, it logs a warning and launches it.

I believe that if we use . as a namespace separator, this will not be possible.

I'd say : or :: are my favorite suggestions from your list. What's your take on that ?

With the point_up in mind it seems that only a task name should be prefixed with a namespace, and that it's the users responsibility to include any namespaces in the alias. Does this seem right?

We could allow blueprint providers to include {namespace} in their aliases and compute that at registration time.

tomdottom · 2021-07-27T14:58:41Z

@ewjoachim

As of today, within a worker, if a task is not registered, Procrastinate makes a last chance attempt and tries to load the Python object pointed at that dotted path. If it's a Procrastinate tasks, it logs a warning and launches it.

So you're saying that as long as a task package is installed Procrastinate will dynamically load the module and look for the task by name? How does the import_paths argument to the App affect this behaviour?

Furthermore, if there is a python module/package which has side-effects at import these could be triggered by creating a task of the module/package name. Part of my 🧠 is telling me that this is undesirable from a security position.

🤔 in a local setup would a task named antigravity open the browser?

tomdottom · 2021-07-27T15:05:44Z

We could allow blueprint providers to include {namespace} in their aliases and compute that at registration time.

Doesn't this defeat the main use case of aliases? As far as I understand an alias is a prior name of a task, so that you can change the name of a task, and will have workers correct run deferred tasks on the queue with the previous name?

I would have thought any aliases would have to be fully qualified with no manipulation done on them.

For instance if a blueprint added a namespace to aliases it would be difficult to transition a task from:

a non-namespaced blueprint to a namespaced blueprint
one namespace to another namespace

This could be worked around if there were someway to opt out of namespacing aliases. But this just strikes me as adding complexity for the sake of a feature that we don't know we need?

tomdottom · 2021-07-27T15:10:24Z

I'd say : or :: are my favorite suggestions from your list. What's your take on that ?

I guess the only thing I have against both of them is they look a little like the syntax for slicing in Python.

Also, considering nested blueprints, would you end up with nested namespaces, sayns1::ns2::my.task?

Some other thoughts:

<ns>my.task
<ns1/ns2>my.task
ns#my.task
ns1#ns2#my.task

ewjoachim · 2021-07-27T16:01:53Z

thinking in a local setup would a task named antigravity open the browser?

I believe it might be interesting to change this :D

tests/unit/test_task_creation_signatures.py

tomdottom · 2021-07-28T17:25:47Z

procrastinate/tasks.py

@@ -81,7 +81,7 @@ def __init__(
        self,
        func: Callable,
        *,
-        app: app.App,
+        app: Optional[app.App],


@ewjoachim this is needed to avoid mypy complaints when a Blueprint creates a task with app = None.

I'm not 100% sure if there are any 2nd/3rd order implications from this, hence bringing it to your attention.

All tests pass and mypy has now stopped complaining about it 🤷‍♂️

codecov · 2021-07-29T20:16:23Z

Codecov Report

Merging #423 (e906197) into master (0780181) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          28       30    +2     
  Lines        1598     1630   +32     
  Branches      175      178    +3     
=======================================
+ Hits         1596     1628   +32     
  Misses          1        1           
  Partials        1        1

Impacted Files	Coverage Δ
procrastinate/__init__.py	`100.00% <100.00%> (ø)`
procrastinate/app.py	`100.00% <100.00%> (ø)`
procrastinate/blueprints.py	`100.00% <100.00%> (ø)`
procrastinate/exceptions.py	`100.00% <100.00%> (ø)`
procrastinate/protocols.py	`100.00% <100.00%> (ø)`
procrastinate/tasks.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0780181...e906197. Read the comment docs.

ewjoachim · 2021-07-29T20:20:58Z

Ok

it looks like we're missing a bit of coverage (in addition to the missing line from Back to 100% coverage #403)
There seems to be a problem with the docs, but let's merge Update deps & fix docs #426 and see what gives.
I'll have to do a final review at some point
And then merge !

tomdottom · 2021-07-30T00:09:38Z

@ewjoachim tests pushed which should get coverage back up

I'll merge/rebase off master once #426 has been merged in.

tomdottom · 2021-07-30T18:46:53Z

Docs finally building correctly.

Enables collections of tasks to be defined independently of the app itself and registered at a later time.

As we can now define tasks with App.task and Blueprint.task it's important to keep the signatures of these to in sync. This commit adds a test to raise and divergence in signatures via pytest. It also adds a new Protocol so mypy can also detect signature divergence.

Protocol was not available until later version of python3.7. We currently support 3.6 and up.

ewjoachim

This starts looking great !
(Ok, I admit I have one thing or 2 I'd like to change but I can do it in a subsequent PR, not to bother you with all my requests :D)

At any point if you'd like me to merge, I'll do it, and I can take care of what's left :)

docs/howto/blueprints.rst

ewjoachim · 2021-07-30T23:04:53Z

docs/howto/blueprints.rst

+
+    app = App(connector=AiopgConnector())
+
+    app.register_blueprint(bp)


Suggested change

app.register_blueprint(bp)

app.register_blueprint(blueprint=blueprint)

The API doesn't require a kwarg, and seeing the word blueprint 3 times in succession just makes me dizzy 😵

docs/howto/blueprints.rst

procrastinate/blueprints.py

ewjoachim · 2021-07-30T23:15:13Z

procrastinate/blueprints.py

+    A Blueprint provides a way to declare tasks that can be registered on an
+    :class:`~procrastinate.App` later::
+
+        bp = Blueprint()


Suggested change

bp = Blueprint()

blueprint = Blueprint()

procrastinate/blueprints.py

ewjoachim · 2021-07-30T23:23:07Z

procrastinate/tasks.py

@@ -167,6 +167,9 @@ def configure(
        ValueError
            If you try to define both schedule_at and schedule_in
        """
+        if self.app is None:
+            raise AssertionError("Tried to configure task whilst self.app was None")


(let's raise a custom exception here)

Created UnboundTaskError

tomdottom · 2021-07-31T17:57:35Z

@ewjoachim

This starts looking great !
(Ok, I admit I have one thing or 2 I'd like to change but I can do it in a subsequent PR, not to bother you with all my requests :D)

At any point if you'd like me to merge, I'll do it, and I can take care of what's left :)

Addressed many of these remaining comments. Please re-review and let me know what's left :)

ewjoachim · 2021-07-31T18:59:49Z

procrastinate/blueprints.py

@@ -20,7 +20,7 @@ class Blueprint(protocols.TaskCreator):

    Notes
    -----
-    Calling a blueprint task before the it is bound to an app will raise an
+    Deffering a blueprint task before the it is bound to an app will raise an


(Deferring, not deffering, but I'll fix it, don't worry)

ewjoachim · 2021-07-31T19:05:11Z

I'm going to merge now, I'll be glad to continue working this feature with you (e.g. for namespaces) but we can do it in a subsequent PR :) At least, we're starting from somewhere :)

tomdottom marked this pull request as ready for review July 26, 2021 19:01

tomdottom requested review from CorBott, DainDwarf, elemoine, ewjoachim, pmourlanne, sophie-ulti and thomasperrot as code owners July 26, 2021 19:01

tomdottom force-pushed the feat/blueprints branch from dc8db66 to c4fb90e Compare July 26, 2021 19:02

tomdottom force-pushed the feat/blueprints branch 2 times, most recently from 4715700 to 7829653 Compare July 26, 2021 19:12

tomdottom mentioned this pull request Jul 26, 2021

Declare tasks independently of an App #421

Closed

ewjoachim approved these changes Jul 27, 2021

View reviewed changes

ewjoachim reviewed Jul 28, 2021

View reviewed changes

tests/unit/test_task_creation_signatures.py Outdated Show resolved Hide resolved

tomdottom commented Jul 28, 2021

View reviewed changes

tomdottom added 2 commits July 30, 2021 14:49

Add Blueprint and lazy registration pattern

627f170

Enables collections of tasks to be defined independently of the app itself and registered at a later time.

Add docs for Blueprints

53f64e7

tomdottom added 9 commits July 30, 2021 14:49

Add Blueprint to top level module

68a7db5

Fix: Correctly register blueprint tasks on app

88a357f

Add acceptance test for Blueprint

12a4350

Fix: Allow Task to have an Optional app attribute

a7f61c7

test: Add implicit task creation test for blueprints

8fe07cb

test: Test for Assertion error if Task in not bound to app

0a0119c

chore: Add docstrings to Blueprint and Blueprint.task

b782f22

fix: Use typing_extensions.Protocol

03faed9

Protocol was not available until later version of python3.7. We currently support 3.6 and up.

tomdottom force-pushed the feat/blueprints branch from 2e5a57a to 03faed9 Compare July 30, 2021 18:49

ewjoachim approved these changes Jul 30, 2021

View reviewed changes

tomdottom added 3 commits July 31, 2021 13:11

fix: Test method signatures of classes, not instances

9f84f3d

feat: Compare task method signatures against protocol definition

c38d5fd

docs: Update docs/howto/blueprints.rst

57d0820

tomdottom force-pushed the feat/blueprints branch from 033b127 to 57d0820 Compare July 31, 2021 17:44

tomdottom added 2 commits July 31, 2021 13:48

docs: Update Blueprint docstring

c3ec963

feat: Use custom exception UnboundTaskError

e906197

tomdottom force-pushed the feat/blueprints branch from 7dd02c2 to e906197 Compare July 31, 2021 17:55

ewjoachim reviewed Jul 31, 2021

View reviewed changes

ewjoachim merged commit 446a79b into procrastinate-org:master Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Blueprint and lazy registration pattern #423

Add Blueprint and lazy registration pattern #423

tomdottom commented Jul 26, 2021 •

edited

Loading

tomdottom commented Jul 26, 2021

tomdottom commented Jul 26, 2021

ewjoachim commented Jul 27, 2021

ewjoachim left a comment

ewjoachim Jul 27, 2021

ewjoachim Jul 27, 2021

tomdottom Jul 27, 2021

ewjoachim Jul 27, 2021

tomdottom commented Jul 27, 2021

ewjoachim commented Jul 27, 2021 •

edited

Loading

tomdottom commented Jul 27, 2021

tomdottom commented Jul 27, 2021

tomdottom commented Jul 27, 2021

ewjoachim commented Jul 27, 2021

tomdottom Jul 28, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading

ewjoachim commented Jul 29, 2021 •

edited

Loading

tomdottom commented Jul 30, 2021

tomdottom commented Jul 30, 2021

ewjoachim left a comment

ewjoachim Jul 30, 2021

tomdottom Jul 31, 2021

ewjoachim Jul 30, 2021

ewjoachim Jul 30, 2021

tomdottom Jul 31, 2021

tomdottom commented Jul 31, 2021

ewjoachim Jul 31, 2021 •

edited

Loading

ewjoachim commented Jul 31, 2021


		app = App(connector=AiopgConnector())

		app.register_blueprint(bp)

	app.register_blueprint(bp)
	app.register_blueprint(blueprint=blueprint)

Add Blueprint and lazy registration pattern #423

Add Blueprint and lazy registration pattern #423

Conversation

tomdottom commented Jul 26, 2021 • edited Loading

Successful PR Checklist:

tomdottom commented Jul 26, 2021

tomdottom commented Jul 26, 2021

ewjoachim commented Jul 27, 2021

ewjoachim 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

tomdottom commented Jul 27, 2021

ewjoachim commented Jul 27, 2021 • edited Loading

tomdottom commented Jul 27, 2021

tomdottom commented Jul 27, 2021

tomdottom commented Jul 27, 2021

ewjoachim commented Jul 27, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

ewjoachim commented Jul 29, 2021 • edited Loading

tomdottom commented Jul 30, 2021

tomdottom commented Jul 30, 2021

ewjoachim 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

tomdottom commented Jul 31, 2021

ewjoachim Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

ewjoachim commented Jul 31, 2021

tomdottom commented Jul 26, 2021 •

edited

Loading

ewjoachim commented Jul 27, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

ewjoachim commented Jul 29, 2021 •

edited

Loading

ewjoachim Jul 31, 2021 •

edited

Loading