-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
[bug] missing 'memcache' dependency in 1.0.0b2 #205
Comments
Try: |
Aha, thanks @Nusnus ! I have a package that depends on |
Ok the It is still a bit unusual compared to other pytest plugins like What use case does |
So this means |
Hey @woutdenolf , this is indeed a subject I am very happy to discuss about, so thank you for bringing up this subject. So, originally, I was under the same perception you suggested, where
Let me expand on my PR description/comment.
A production environment may have infinite combinations of tech stacks, which may include official and/or modified Celery workers clusters. Such production environments have their own specific dependencies and complexities. So, to avoid even attempting to take responsibility for that, the design choice was to install the
Another very common use-case would be to “just get my EVERYTHING ready one-click no stop” to POC ideas and test more simple Celery use-cases, with out-of-the-box setup. For that, the
Great point. The design choice for using the
Based on the explanation above then, if you use the For example, the new smoke tests are installing
The smoke tests environment defines a smoke tests worker that does NOT use the built-in default latest Celery version worker from the plugin but instead uses the “production-style” of taking responsibility for the worker installation by itself to make sure the installed Celery version is done using edit mode on the current source code. RUN pip install --no-cache-dir --upgrade \
pip \
-e /celery[redis,pymemcache] \
psutil Where the source code is loaded via a volume: volumes={
# Volume: Worker /app
"{default_worker_volume.name}": defaults.DEFAULT_WORKER_VOLUME,
# Mount: Celery source
os.path.abspath(os.getcwd()): {
"bind": "/celery",
"mode": "rw",
},
}, So to reiterate,
This means then that you’re responsible for installing the packages your environment uses so as to match testing/tested package installation. If you’re installing without the celery extra, DO install the dependencies you use (e.g redis), don’t test for the memcached backend configuration (disabled by default right now), and still get the missing module error then there’s a real bug, which correctly matches this issue’s subject so I will also reopen this issue until we can come to understanding or new conclusions. I hope I made it all clear - feel free to ask anything. I welcome any productive feedback! |
@woutdenolf, does my reasoning make sense to you? |
That is exactly the problem. Or vice versa, a project has
Agreed. I just couldn't find a way to do it with poetry but them found python-poetry/poetry-core#613 (comment).
Still I don't understand why having
Correct me if I'm wrong, but I see no problem here. Let me put it differently, is there a way in which we can make celery importable from celery import Celery and then I guess the only way this could be done is making celery importable without pip installing it. For example by using the PYTHONPATH
So even this won't install celery. It does install missing celery core dependencies. But I don't consider this a legitimate use case. Then there is the upper limit on the celery version
Could this cause problems? Suppose we have this
But if |
My assumption is: Yes. This is why wanted the The fix should be configuring the plugin to your setup by code (
Needs to be removed indeed. Good point! Now regarding, and
Ok, so let’s see how we deal with this forward.
Let me expand on that, So we have RabbitMQ, Redis and Memcached vendors. *Component is a docker container and a node controller.
Where is the design problem then IMHO? API, interface. This is not intuitive, nor the best implementation for the given goal, I agree. So, let me ask what would you say about this interface:
Also maybe if it’s not too much,
Which can all be bundled alongside the Lastly, this means the error handling needs to be developed for better package handling/error messages. I would like to attempt a broad and more thorough implementation of the fix, which might be out of the scope of your PR (might require some deeper/inner design changes in other places) - would it be alright with you @woutdenolf if I close the PR then and work on the implementation alongside our conversation here? EDIT:
This indeed follows the explanation above, where COPY --chown=test_user:test_user . /celery
RUN pip install --no-cache-dir --upgrade \
pip \
-e /celery[redis,pymemcache] \ |
I'll have to see a real use case to be convinced. Anyway, the projects that use
Lets think of from celery import Celery
from kombu import Connection
from redis import StrictRedis
import memcache So all of these are dependencies, optional or required but dependencies regardless. When none of the optional dependencies are installed, these import statements should not fail imo import pytest_celery # if this fails, you cannot use pytest-celery without the optional dependencies
import pytest_celery.plugin # if this fails, you cannot use pytest without the optional dependencies I believe the real design issue is that everything is imported in the main In my PR I captured the import pytest_celery
import pytest_celery.plugin Then when the tests of other projects actually trying to use the optional dependencies, an exception gets raised at that moment. Then in terms of the project extra's. Again lets think of the projects we import: pip install pytest-celery[celery,redis,memcache] # strictly speaking rabbitmq needs to be in here as well which we use kombu for In projects with many optional dependencies I often add the "full" extra to install all of them (which is the equivalent of what "celery" currently does) pip install pytest-celery[full] That being said I'm also ok not having any of the individual optional dependencies and only keep "full" or "celery" as it is now called. So basically leave things the way they are (perhaps fixing the celery version thing). The important part is not breaking
Sure. In the end if I can use |
I’ve decided to take a different angle in approaching this issue. first,
second,
When I re-review this subject from your perspective, I start to make sense of what’s wrong with my reasoning.
So, to accommodate the above reasoning into priorities, I’ve decided to revert the Lastly, the design flaw can be solved separately, making sure the problem this discussion raised are not repeating, allowing focusing at each issue at a time, where it hurts more first :) |
I found a case for my original reasoning 🤦♂️😅 pip install pip install pytest-celery==1.0.0b1
pip install celery==4.4.7
…
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
pytest-celery 1.0.0b2 requires celery[pymemcache,redis]<6.0.0,>=5.0.0, but you have celery 4.4.7 which is incompatible. When using pip install pytest-celery==1.0.0b2
pip install celery==4.4.7 Works. This means the idea behind removing the This still doesn’t mean we can just go back to making it optional as before because of the design flaw we discussed. I’m beginning to believe there is no escape other than a proper fix.. Oh well.. |
Well you can install this but it is not expected to work because pytest-celery If pytest-celery |
It should support any version in theory as one of the main use-cases of the plugin will be assisting migrating to new Celery versions, so it needs to allow hybrid versions at the same time. That being said, I'm still not sure what's the best solution. How to implement it with poetry later will be the easy part. |
This works
This fails
celery 5.3.6 and pytest 8.0.0 in both cases.
The text was updated successfully, but these errors were encountered: