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

[docs] sensors docs: clarify run_key behavior in a single sensor evaluation #26404

Closed
wants to merge 1 commit into from

Conversation

Gw1p
Copy link

@Gw1p Gw1p commented Dec 11, 2024

Summary & Motivation

I was caught off-guard in a production environment when a sensor that yields multiple RunRequests with the same run_key didn't de-duplicate them. Based on the current phrasing of the documentation, I would've thought that it would possibly take the first RunRequest and only launch that one but that doesn't seem to be the case.

I think it's better for the documentation to explicitly state what is the expected behavior in this scenario (assuming the current behavior is intentional).

How I Tested These Changes

Straightforward documentation change. Ran cd docs; cd next; yarn dev to spin up the docs locally and checked that it looks fine.

Changelog

Specified run_key behavior when emitting multiple RunRequests with the same run_key within a single sensor evaluation

@Gw1p Gw1p requested a review from neverett as a code owner December 11, 2024 15:08
@Gw1p Gw1p marked this pull request as draft December 11, 2024 15:11
@Gw1p Gw1p changed the title sensors docs: clarify run_key behavior in a single sensor evaluation Draft: sensors docs: clarify run_key behavior in a single sensor evaluation Dec 11, 2024
@Gw1p Gw1p changed the title Draft: sensors docs: clarify run_key behavior in a single sensor evaluation sensors docs: clarify run_key behavior in a single sensor evaluation Dec 11, 2024
@Gw1p Gw1p marked this pull request as ready for review December 11, 2024 16:54
@Gw1p Gw1p changed the title sensors docs: clarify run_key behavior in a single sensor evaluation [docs] sensors docs: clarify run_key behavior in a single sensor evaluation Dec 11, 2024
@neverett
Copy link
Contributor

I chatted with our engineering team and it sounds like this might not be supported behavior /cc @danielgafni

@Gw1p
Copy link
Author

Gw1p commented Dec 17, 2024

I chatted with our engineering team and it sounds like this might not be supported behavior /cc @danielgafni

Not supported in a sense that you're saying it doesn't currently work that way or in a sense that it shouldn't? If it's the former, happy to provide a minimum example. If the latter, happy to close the PR and file a bug instead.

An example would look something like:

@op
def dummy_op(context):
    context.log.info("Hello")
    yield Output(None)


@job
def dummy_job():
    dummy_op()


@sensor(
    job=dummy_job
)
def dummy_sensor(context):
    for i in range(3):
        yield RunRequest(
            run_key="5",
        )

On the first tick evaluation, it will launch 3 dummy_jobs, each with run_key of 5.

@neverett
Copy link
Contributor

Not supported in a sense that you're saying it doesn't currently work that way or in a sense that it shouldn't? If it's the former, happy to provide a minimum example. If the latter, happy to close the PR and file a bug instead.

In the sense that it shouldn't, so feel free to file a bug, thanks!

@danielgafni
Copy link
Contributor

I think this ping was supposed to be for @gibsondan?

@neverett
Copy link
Contributor

I think this ping was supposed to be for @gibsondan?

Yes, my apologies!

@Gw1p
Copy link
Author

Gw1p commented Dec 20, 2024

Not supported in a sense that you're saying it doesn't currently work that way or in a sense that it shouldn't? If it's the former, happy to provide a minimum example. If the latter, happy to close the PR and file a bug instead.

In the sense that it shouldn't, so feel free to file a bug, thanks!

Gotcha, will do! I'm away for Christmas holidays until Thursday; will get a good example for it and file a bug once I'm back. Happy to look into fixing it as well.

@Gw1p
Copy link
Author

Gw1p commented Dec 28, 2024

Opened issue here: #26753

Closing the PR

@Gw1p Gw1p closed this Dec 28, 2024
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