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

pipe_ignore ignored as part of actor options #100

Closed
dmitry-vakhnenko opened this Issue Jul 6, 2018 · 5 comments

Comments

3 participants
@dmitry-vakhnenko

dmitry-vakhnenko commented Jul 6, 2018

Issues

GitHub issues are for bugs. If you have questions, please ask them on the discussion board.

Checklist

  • Does your title concisely summarize the problem?
  • Did you include a minimal, reproducible example?
  • What OS are you using?
  • What version of Dramatiq are you using?
  • What did you do?
  • What did you expect would happen?
  • What happened?

What OS are you using?

Fedora 28

What version of Dramatiq are you using?

1.2.0

What did you do?

broker = RedisBroker(connection_pool=redis_pool)
results_backend = RedisBackend(connection_pool=redis_pool)
broker.add_middleware(Results(backend=results_backend, store_results=True))

class BaseTask(dramatiq.GenericActor):
    class Meta:
        abstract = True
        max_retries = 3
        max_age = 120_000
        retry_when = retry_when
        pipe_ignore = True

class HourlyReportTask(BaseTask):
    def perform(self, access_token, account_id, posts_limit=4):
        client = create_client(access_token)

        report = Report.create(account_id=account_id, report_type="hourly")

        args = (report.id, access_token, account_id)

        messages = [
            ProfileReportTask.message(*args),
            ProfileInsightsReportTask.message(*args),
        ]

        posts = client.get(
            f"{account_id}/media", access_token=access_token, limit=posts_limit
        )

        for post in posts["data"]:
            messages.append(
                MediaReportTask.message(report.id, access_token, post["id"])
            )

        g = dramatiq.pipeline(messages).run()
        g.wait()

        now = pendulum.now(tz="UTC")
        Report.update(completed_at=now).where(Report.id == report.id).execute()

What did you expect would happen?

Setting pipe_ignore = True on BaseTask should pass this option to all childs of BaseTask automatically.

What happened?

TypeError: perform() takes 4 positional arguments but 5 were given

In my case i should create message for every task with message_with_options(pipe_ignore=True) or implementing same as Results middleware.

@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Jul 15, 2018

Thanks! This will be fixed in the next release.

@Bogdanp Bogdanp self-assigned this Jul 15, 2018

@Bogdanp Bogdanp added the bug label Jul 15, 2018

@Bogdanp Bogdanp added this to the v1.3.1 milestone Jul 15, 2018

@Bogdanp Bogdanp changed the title from pipe_ignore ignores on with Class based actors to pipe_ignore ignored as part of actor options Jul 15, 2018

@Bogdanp Bogdanp modified the milestones: v1.3.1, v1.4.0 Oct 21, 2018

@ryanhiebert

This comment has been minimized.

Contributor

ryanhiebert commented Nov 22, 2018

This is an interesting question. What if Meta were defined on both classes, but with different keys? Should the keys from the base class be included in the meta for the subclass, or should those revert to the defaults? Having it inherit isn't native Python, but it also makes a lot of sense as a reasonable feature.

@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Nov 22, 2018

@ryanhiebert

Should the keys from the base class be included in the meta for the subclass, or should those revert to the defaults?

The former is how it works. Meta is inherited by subclasses. The issue here is different, though: the middleware does not (didn't, actually) look at the actor-level pipe_ignore option.

@ryanhiebert

This comment has been minimized.

Contributor

ryanhiebert commented Nov 22, 2018

Oh cool, thanks for clearing that up, I read this as an inheritance issue.

@Bogdanp

This comment has been minimized.

Owner

Bogdanp commented Nov 24, 2018

This is now in master and will be released soon!

@Bogdanp Bogdanp closed this Nov 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment