harvard-lil / h2o

H2O is a web app for creating and reading open educational resources, primarily in the legal field
https://opencasebook.org
GNU Affero General Public License v3.0
37 stars 30 forks source link

Replace Fabric with Invoke #1916

Closed bensteinberg closed 1 year ago

bensteinberg commented 1 year ago

Since we only use Fabric to run things locally, we can switch to Invoke, removing a few dependencies. Closes #1810.

Notable changes for developers who use Invoke tasks include replacing underscores with hyphens: invoke run-frontend instead of fab run_frontend -- and boolean arguments are really boolean now: invoke run --debug-toolbar instead of fab run:debug_toolbar=True

When writing new Invoke tasks, the two things to remember are including the context as the first argument, and that arguments are not necessarily strings, as they are in Fabric3 -- see https://docs.pyinvoke.org/en/stable/concepts/invoking-tasks.html#type-casting

See also https://github.com/harvard-lil/perma-payments/pull/165 -- there's some discussion there about whether and how to change the names of files in which Invoke and Celery look for tasks. The short version is that we are going to let the less-configurable Invoke have tasks.py, and I've chosen to give Celery celery_tasks.py.

This PR also updates the @sentry/tracing JS package, and removes a PDF file generated in testing that snuck in a few months ago.

lizadaly commented 1 year ago

It looks like this'll be a nice improvement (I especially like that it can use real command-line args).

The celery change wasn't happy though:

[2023-02-24 21:16:24,144: ERROR/MainProcess] Received unregistered task of type 'main.tasks.demo_scheduled_task'.
The message has been ignored and discarded.

Did you remember to import the module containing this task?
Or maybe you're using relative imports?

Please see
http://docs.celeryq.org/en/latest/internals/protocol.html
for more information.

The full contents of the message body was:
b'[[], {}, {"callbacks": null, "errbacks": null, "chain": null, "chord": null}]' (77b)

Thw full contents of the message headers:
{'lang': 'py', 'task': 'main.tasks.demo_scheduled_task', 'id': '5d8e8cfe-c5dc-44c2-931f-e0d4a3547382', 'shadow': None, 'eta': None, 'expires': None, 'group': None, 'group_index': None, 'retries': 0, 'timelimit': [None, None], 'root_id': '5d8e8cfe-c5dc-44c2-931f-e0d4a3547382', 'parent_id': None, 'argsrepr': '()', 'kwargsrepr': '{}', 'origin': 'gen33@1454fbf2a403', 'ignore_result': False}

The delivery info for this task is:
{'exchange': '', 'routing_key': 'background'}
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/celery/worker/consumer/consumer.py", line 591, in on_task_received
    strategy = strategies[type_]
KeyError: 'main.tasks.demo_scheduled_task'

Maybe a simple solution would be to set an alias in the Docker user's .bashrc?

e.g.:

alias invoke="invoke -c invoke_tasks"

and then leave tasks.py to celery? Alternatively update any references to main.tasks.

lizadaly commented 1 year ago

Looks like you fixed it while I was still babbling!

codecov-commenter commented 1 year ago

Codecov Report

Merging #1916 (a6e0c37) into develop (aa052b5) will increase coverage by 0.03%. The diff coverage is 67.74%.

@@             Coverage Diff             @@
##           develop    #1916      +/-   ##
===========================================
+ Coverage    76.82%   76.86%   +0.03%     
===========================================
  Files           56       56              
  Lines         6731     6725       -6     
===========================================
- Hits          5171     5169       -2     
+ Misses        1560     1556       -4     
Impacted Files Coverage Δ
web/main/celery_tasks.py 47.36% <ø> (ø)
web/tasks.py 23.54% <64.28%> (ø)
web/config/celery.py 100.00% <100.00%> (ø)
web/main/test/functional/test_platform.py 57.81% <100.00%> (ø)
web/main/views.py 71.49% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lizadaly commented 1 year ago
inv run

This works, but other tasks aren't found:

root@1454fbf2a403:/app/web# inv run_frontend
No idea what 'run_frontend' is!

root@1454fbf2a403:/app/web# inv refresh_search_index
No idea what 'refresh_search_index' is!

On first glance I can't see why.

lizadaly commented 1 year ago

I'm an idiot!

bensteinberg commented 1 year ago

Yeah, I will update deployment config before I merge this.