Skip to content
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

[24.1] Remove the default Incoming suffix in GenericModel class #19174

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Nov 20, 2024

Fixes:

Traceback (most recent call last):
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/worker.py", line 202, in start
    self.blueprint.start(self)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/bootsteps.py", line 116, in start
    step.start(parent)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/bootsteps.py", line 365, in start
    return self.obj.start()
           ^^^^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/consumer/consumer.py", line 340, in start
    blueprint.start(self)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/bootsteps.py", line 116, in start
    step.start(parent)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/consumer/consumer.py", line 746, in start
    c.loop(*c.loop_args())
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/loops.py", line 97, in asynloop
    next(loop)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/kombu/asynchronous/hub.py", line 373, in create_loop
    cb(*cbargs)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/kombu/transport/base.py", line 248, in on_readable
    reader(loop)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/kombu/transport/base.py", line 230, in _read
    drain_events(timeout=0)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/amqp/connection.py", line 526, in drain_events
    while not self.blocking_read(timeout):
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/amqp/connection.py", line 532, in blocking_read
    return self.on_inbound_frame(frame)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/amqp/method_framing.py", line 77, in on_frame
    callback(channel, msg.frame_method, msg.frame_args, msg)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/amqp/connection.py", line 538, in on_inbound_method
    return self.channels[channel_id].dispatch_method(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/amqp/abstract_channel.py", line 156, in dispatch_method
    listener(*args)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/amqp/channel.py", line 1629, in _on_basic_deliver
    fun(msg)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/kombu/messaging.py", line 656, in _receive_callback
    return on_m(message) if on_m else self.receive(decoded, message)
           ^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/consumer/consumer.py", line 685, in on_task_received
    strategy(
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/strategy.py", line 207, in task_message_handler
    handle(req)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/worker.py", line 220, in _process_task_sem
    return self._quick_acquire(self._process_task, req)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/kombu/asynchronous/semaphore.py", line 75, in acquire
    callback(*partial_args, **partial_kwargs)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/worker.py", line 225, in _process_task
    req.execute_using_pool(self.pool)
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/worker/request.py", line 754, in execute_using_pool
    result = apply_async(
             ^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/concurrency/base.py", line 153, in apply_async
    return self.on_apply(target, args, kwargs,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/galaxy/venv/lib/python3.11/site-packages/billiard/pool.py", line 1527, in apply_async
    self._quick_put((TASK, (result._job, None, func, args, kwds)))
  File "/opt/galaxy/venv/lib/python3.11/site-packages/celery/concurrency/asynpool.py", line 868, in send_job
    body = dumps(tup, protocol=protocol)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_pickle.PicklingError: Can't pickle <class 'galaxy.schema.notifications.NotificationRecipientsIncoming'>: attribute lookup NotificationRecipientsIncoming on galaxy.schema.notifications failed

Notice that this happens only with certain Python/Celery/Pydantic version combinations (i.e. Python3.11.5 / Celery5.4.0 / Pydantic2.7.4).

This issue was crashing every worker trying to pick up the task with a generic model in the parameters. In particular, send_notification_to_recipients_async.

The NotificationRecipientsIncoming dynamically created model from the generic is not yet available in the module.

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot changed the title Remove the default Incoming suffix in GenericModel class [24.1] Remove the default Incoming suffix in GenericModel class Nov 20, 2024
@github-actions github-actions bot added this to the 24.1 milestone Nov 20, 2024
@mvdbeek
Copy link
Member

mvdbeek commented Nov 20, 2024

Were you able to reproduce this locally ? Could you test pydantic/pydantic#9390 (comment) ?

@davelopez
Copy link
Contributor Author

I was able to reproduce locally with Python 3.11.5
I could test that patch too, I saw it before but decided to remove the suffix instead of adding more patch code. But I will test this patch locally and report back so we can decide 👍

@mvdbeek
Copy link
Member

mvdbeek commented Nov 20, 2024

I think the mixin looks a lot less likely to cause problems than relying on a mismatch in the globals ...

@davelopez davelopez force-pushed the 24.1_fix_pickle_issue_in_celery_notifications branch from e5f1123 to f96111a Compare November 20, 2024 17:24
@davelopez
Copy link
Contributor Author

Fair enough! That's a good point. I added the mixing and seems to work as expected 👍

@mvdbeek mvdbeek merged commit c290421 into galaxyproject:release_24.1 Nov 21, 2024
49 of 51 checks passed
@davelopez davelopez deleted the 24.1_fix_pickle_issue_in_celery_notifications branch November 21, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants