Skip to content

Back port __wrapped__ attribute for functools.wrap to Python 2 #475

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

Closed
wants to merge 1 commit into from

Conversation

cameronmaske
Copy link

No description provided.

@untitaker
Copy link
Member

Where do you need this?

@cameronmaske
Copy link
Author

cameronmaske commented Aug 20, 2019

@untitaker My apologies, I accidentally opened this PR and isn't ready yet.

I'm the author of celery-once and this PR is related to #421.

For context, behind the scenes, celery once attempts to inspect the arguments of the task function to generate a "key" to lock the task.
While #432 fixed the issue for Python 3, for Python 2, getcallargs can't inspect the original function, as the __wrapped__ attribute (which stores the original function) was only added in >3.2.
This is an attempt to backport that functionality for Python 2.

@untitaker
Copy link
Member

Got it. Conceptually I would approve this PR. Could you add a test that loads and uses celery-once? Also please move this new wraps into sentry_sdk.utils.

Ping me when it's ready :)

@cameronmaske cameronmaske changed the title Py2 wrapped Back port __wrapped__ attribute for functools.wrap to Python 2 Aug 21, 2019
@cameronmaske
Copy link
Author

Hi @untitaker the code should be ready for a review.

I'm not sure exactly why the tests are failing on Travis failed, any insight?

image

image

…ameronmaske/celery-once/issues/105)

Add tests for celery_once.
Move re-usable pytest fixture for celery into a common conftest.py.
@cameronmaske
Copy link
Author

@untitaker Thanks for the advice on the tests! Your suggestion are implemented and this should be ready for re-review.

@untitaker
Copy link
Member

@cameronmaske I don't quite understand the test, but it still seems to pass if I revert all your changes in sentry_sdk. So I think it isn't effective in testing that the bug does not exist.

@untitaker
Copy link
Member

Also sorry for the slow review!

@cameronmaske
Copy link
Author

cameronmaske commented Aug 27, 2019

@untitaker Just to check, with the reverted changes, were the tests run on Python 3 or 2? From my understanding, the tests should still pass on Python 3 without the changes, but fail on 2.

The tests are a bit obtuse, but they are checking that backend is called with a correctly generated key from the example's tasks name and arguments.
i.e.

def dummy_task(x, y):
   pass

Should become

qo_tests.integrations.celery.test_celery_once.dummy_task_x-1_y-1
# < Name assigned by celery to task> . (<Argument Key > <Arugment Value>, <Argument Key> <Arugment Value>,....

In this case, I put a dummy backend (simpler too test), but normally, this would be a redis or file backend to check the locks.

@untitaker
Copy link
Member

I was running this:

git checkout origin/master sentry_sdk/
tox -e py2.7-celery-3

...since you only use celery-once in celery 3.

@untitaker
Copy link
Member

@cameronmaske any update on this? Right now the tests don't seem useful IMO

@cameronmaske
Copy link
Author

I need to investigate this further on my end. I'm currently travelling and away from my development environment, will follow up when I return later next week.

@Marcelo-Theodoro
Copy link

Just for clarification: This changes still will be needed? Even after upgrading celery-once to 3.0.1?

If so, anything I can help here?

Thanks.

@untitaker
Copy link
Member

@Marcelo-Theodoro I would love it if somebody took over this PR and fixed merge conflicts + fixed the tests to actually fail when the bugfix is not applied

I can't answer the first question, this is something for @cameronmaske

@cameronmaske
Copy link
Author

@untitaker Sorry for the delayed follow up. Had a chance to test this, and it appears that switching from getcallargs -> signature the issue wraps does not occur. I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants