Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Batch submit task #41

Merged
merged 11 commits into from
Jun 24, 2022
Merged

Batch submit task #41

merged 11 commits into from
Jun 24, 2022

Conversation

ahuang11
Copy link
Contributor

Adds #38

@ahuang11 ahuang11 requested a review from desertaxle as a code owner June 15, 2022 17:52
@desertaxle desertaxle linked an issue Jun 22, 2022 that may be closed by this pull request
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

batch_submit looks good. I think it's important to also have the waiting functionality that's mentioned in #38. Can you also add a client_waiter task in this PR or in a follow up PR?

@ahuang11
Copy link
Contributor Author

Perhaps another PR, and I think @mtconger97 might be interested in helping out with it!

@ahuang11 ahuang11 requested a review from desertaxle June 24, 2022 18:23
Comment on lines 8 to 12
@pytest.fixture(scope="session", autouse=True)
def prefect_db():
with prefect_test_harness():
yield

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this fixture?

Copy link
Contributor Author

@ahuang11 ahuang11 Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was crashing tests earlier (e.g. from prefect.utilities.testing import prefect_test_harness), but now I think it's not used since the tests are still passing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asyncio: mode=auto
collected 58 items

tests/test_batch.py .                                                                                                                                                                              [  1%]
tests/test_client_parameters.py ....                                                                                                                                                               [  8%]
tests/test_s3.py ...............                                                                                                                                                                   [ 34%]
tests/test_secrets_manager.py ......................................                                                                                                                               [100%]

===================================================================================== 58 passed in 103.01s (0:01:43) =====================================================================================

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the comments in #27 where it was added, it's a nice thing to have to spin up a temporary DB per test. I think we should leave it in and correct the import path (from prefect.test.utilities import prefect_test_harness).

@ahuang11 ahuang11 requested a review from desertaxle June 24, 2022 19:12
@ahuang11 ahuang11 merged commit d4c6ae5 into main Jun 24, 2022
@discdiver discdiver deleted the batch_submit_task branch August 1, 2023 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants