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.

Sign up for GitHub

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

Jump to bottom

Blueprints get duplicate namespace prefixes in tests #521

Open
kbakk opened this issue Jan 6, 2022 · 3 comments · May be fixed by #524
Open

Blueprints get duplicate namespace prefixes in tests #521

kbakk opened this issue Jan 6, 2022 · 3 comments · May be fixed by #524
Labels
Issue appropriate for: People up for a challenge 🤯 This issue probably will be challenging to tackle Issue contains: Exploration & Design decisions 🧩 We don't know how this will be implemented yet Issue contains: Some Python 🐍 This issue involves writing some Python code Issue type: Bug 🐞 Something isn't working

Comments

@kbakk
Copy link

kbakk commented Jan 6, 2022

I am initializing the app several times in my tests, and after upgrading to 0.23.0 I noticed that the tests have started to fail, with procrastinate.exceptions.TaskNotFound:

ERROR [procrastinate.worker] Task was not found: 
    Task cannot be imported.
    
Traceback (most recent call last):
  File "/Users/krisb/Library/Caches/pypoetry/virtualenvs/file-processing-trigger--C-Yay8K-py3.9/lib/python3.9/site-packages/procrastinate/worker.py", line 206, in find_task
    return self.app.tasks[task_name]
KeyError: 'mediabank_ingest:trigger_ingest'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/krisb/Library/Caches/pypoetry/virtualenvs/file-processing-trigger--C-Yay8K-py3.9/lib/python3.9/site-packages/procrastinate/worker.py", line 169, in process_job
    await self.run_job(job=job, worker_id=worker_id)
  File "/Users/krisb/Library/Caches/pypoetry/virtualenvs/file-processing-trigger--C-Yay8K-py3.9/lib/python3.9/site-packages/procrastinate/worker.py", line 213, in run_job
    task = self.find_task(task_name=task_name)
  File "/Users/krisb/Library/Caches/pypoetry/virtualenvs/file-processing-trigger--C-Yay8K-py3.9/lib/python3.9/site-packages/procrastinate/worker.py", line 208, in find_task
    raise exceptions.TaskNotFound
procrastinate.exceptions.TaskNotFound: 
    Task cannot be imported.

I noticed when debugging the tests that my tasks had duplicated their prefixes:

image

I have a function scoped fixture that returns the app, which is created this way

def get_worker_app(connector):
    app = procrastinate.App(connector=connector)
    app.open()
    app.add_tasks_from(debug_to_disk.bp, namespace=module_to_namespace(debug_to_disk.__name__))
    app.add_tasks_from(mediabank_ingest.bp, namespace=module_to_namespace(mediabank_ingest.__name__))
    return app

@pytest.fixture
def app():
    app = get_worker_app(connector=InMemoryConnector())
    yield app
    # Reset the "in-memory pseudo-database" between tests:
    app.connector.reset()

So this gets called multiple times. It seems like the blueprint persists the name of the task, including the prefix.

Here's the blueprint:

bp = Blueprint()

@bp.task(name="write_to_file")
def write_to_file(path: str, content: str):
    pathlib.Path(path).write_text(content)
    logger.info(f"Wrote content to: {path}")

I've been able to work around this by having a reset_blueprint fixture, that I'm using now:

@pytest.fixture
def reset_blueprints():
    # rerun imports, to get a reset blueprints, else they will persist data from previous runs
    importlib.reload(file_processing_trigger.tasks.debug_to_disk)
    importlib.reload(file_processing_trigger.tasks.mediabank_ingest)

@pytest.fixture
def app(reset_blueprints):
    ...

I'm not sure if this is a bug, or something else but importlib.reload seems like a workaround.

Sorry about no proper reproducer, let me if that's needed and I can try to set that up.
@ewjoachim
Copy link
Member

Hello !

Thanks for the bug report ! I'm not that surprised to see thins kind of behaviours as the namespacing feature had not been written with the tests in mind. I'll see if I can get a fix out there.

@ewjoachim
Copy link
Member

Here's how it's done in the procrastinate codebase, and it's suboptimal :/

procrastinate/tests/conftest.py

Lines 170 to 175 in a30a514

@pytest.fixture
def reset_builtin_task_names():
builtin_tasks.remove_old_jobs.name = "procrastinate.builtin_tasks.remove_old_jobs"
builtin_tasks.builtin.tasks = {
task.name: task for task in builtin_tasks.builtin.tasks.values()
}

@ewjoachim ewjoachim linked a pull request Jan 8, 2022 that will close this issue
Draft
4 tasks
@ewjoachim
Copy link
Member

Ok, so the problem is that when we register a task, its name is updated with the new name.
This side-effect is problematic, but it's necessary to do task.defer()

One simple (but wrong) solution could be to adapt the utils.add_namespace function to not do anything if the namespace is already present on the function. But I believe it would be the wrong solution.

I believe there might be a solution where we keep a (weak) ref to blueprints & apps in the tasks, and compute the name dynamically depending on what self.blueprint points to.

I've made a failing test, now it's time to brainstorm :)

@ewjoachim ewjoachim added Issue appropriate for: People up for a challenge 🤯 This issue probably will be challenging to tackle Issue contains: Exploration & Design decisions 🧩 We don't know how this will be implemented yet Issue contains: Some Python 🐍 This issue involves writing some Python code Issue type: Bug 🐞 Something isn't working labels Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Issue appropriate for: People up for a challenge 🤯 This issue probably will be challenging to tackle Issue contains: Exploration & Design decisions 🧩 We don't know how this will be implemented yet Issue contains: Some Python 🐍 This issue involves writing some Python code Issue type: Bug 🐞 Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix duplicate prefixes issues procrastinate-org/procrastinate
2 participants
@ewjoachim @kbakk