-
Notifications
You must be signed in to change notification settings - Fork 94
Fix Infrequent Polling sample #150
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
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,14 +1,9 @@ | ||||||
| class TestService: | ||||||
| def __init__(self): | ||||||
| self.try_attempts = 0 | ||||||
| self.error_attempts = 5 | ||||||
|
|
||||||
| async def get_service_result(self, input): | ||||||
| print( | ||||||
| f"Attempt {self.try_attempts}" | ||||||
| f" of {self.error_attempts} to invoke service" | ||||||
| ) | ||||||
| self.try_attempts += 1 | ||||||
| if self.try_attempts % self.error_attempts == 0: | ||||||
| async def get_service_result(self, input, attempt: int): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went a couple turns down this rabbit hole, but it turns out to require a shockingly large refactoring, which is not worth doing. |
||||||
| print(f"Attempt {attempt} of {self.error_attempts} to invoke service") | ||||||
| if attempt > self.error_attempts: | ||||||
| return f"{input.greeting}, {input.name}!" | ||||||
| raise Exception("service is down") | ||||||
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import uuid | ||
|
|
||
| import pytest | ||
| from temporalio.client import Client | ||
| from temporalio.testing import WorkflowEnvironment | ||
| from temporalio.worker import Worker | ||
|
|
||
| from polling.infrequent.activities import compose_greeting | ||
| from polling.infrequent.workflows import GreetingWorkflow | ||
|
|
||
|
|
||
| async def test_infrequent_polling_workflow(client: Client, env: WorkflowEnvironment): | ||
| if not env.supports_time_skipping: | ||
drewhoskins-temporal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pytest.skip("Too slow to test with time-skipping disabled") | ||
|
|
||
| # Start a worker that hosts the workflow and activity implementations. | ||
| task_queue = f"tq-{uuid.uuid4()}" | ||
| async with Worker( | ||
| client, | ||
| task_queue=task_queue, | ||
| workflows=[GreetingWorkflow], | ||
| activities=[compose_greeting], | ||
| ): | ||
| handle = await client.start_workflow( | ||
| GreetingWorkflow.run, | ||
| "Temporal", | ||
| id=f"infrequent-polling-{uuid.uuid4()}", | ||
| task_queue=task_queue, | ||
| ) | ||
| result = await handle.result() | ||
| assert result == "Hello, Temporal!" | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the way the sample previously did not pass the attempt number to the test service is desirable to maintain, since in real world examples the service in question won't want to know about our attempt counting.
In other words, the way it is now will make some readers think that the solution to the polling-from-activity problem involves passing an attempt count to a service.
Is there a way to fix the algorithm without explicitly passing the attempt number to the service?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand and like the meta-point of wanting samples to look real-world, though this one feels shippable anyway. I could use a global, but it wouldn't work out-of-process, and anyway I'm not inspired to spend the time to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I do think it's worth finding a way to make your test pass without making the sample misleading. Here's a global variable version: #152 (I'm starting to see why the code had that modulo-arithmetic return condition!)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly, this doesn't work predictably when it's run multiple times on the same worker. I really don't think anybody's going to be misled by it as it is, and I would greatly prefer predictability and test isolation as a user.
That said, I wouldn't block your approach if you feel strongly enough.
Can you either accept my PR and then add on top of it, or just leave it as I have it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub was having problems with this PR after I brought it up-to-date with
mainand it's closed currently. Are you able to reopen it?The code in this sample falls into two categories:
Code that we are providing to users, essentially saying "follow this closely; this is what your code should look like". Note that users may well copy and paste code in this category. In particular:
workflows.py,activities.pyPrivate implementation detail of the sample, which exists only to make the sample work. Users will not copy and paste this code because, in their use cases, its role will be played by something specific to their domain. In particular:
test_service.pyFor (2) the basic idea of the sample is to create a toy service that is stateful, only responding after a few attempts. But that statefulness needs to be private to the
test_serviceimplementation, since it's modeling real-world slowness-to-come-up. The one thing we don't want to do is give users the impression that they need to count their attempts in their activity code and send them to a service. The sample in its current form takes care not to do that, and we don't need to make the same worse in that respect.There are a few options I think:
test_serviceto the module level and use modulo arithmetic to address the issue that it might be used by multiple workflows.test_service = TestService()inactivities.pyto the top level and use modulo arithmetic to address the issue that it might be used by multiple workflows.activity.info().workflow_idfor the counter intest_serviceAll those would teach the lesson we're trying to teach correctly. (1) and (2) are suboptimal in that they'd result in some potentially confusing shared state if someone were to run multiple instances of the sample concurrently. So (3) is perhaps the best choice, seeing as it's hardly more complex than (1) and (2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented option (3) in #152, which builds upon this PR (i.e. using the test you added).