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

Clean up unused settings/experimental workpool flags PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_WARN_WORK_POOLS #13144

Merged
merged 14 commits into from
May 24, 2024

Conversation

jaraics
Copy link
Contributor

@jaraics jaraics commented Apr 28, 2024

Usage of
PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS were removed in PrefectHQ#8362

There is no reference to them anywhere else, except in settings.py where they are declared. They are just clutter when you open the settings page in the self-hosted server.

Example

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • This pull request includes a label categorizing the change e.g. maintenance, fix, feature, enhancement, docs.

For documentation changes:

  • This pull request includes redirect settings in netlify.toml for files that are removed or renamed.

For new functions or classes in the Python SDK:

  • This pull request includes helpful docstrings.
  • If a new Python file was added, this pull request contains a stub page in the Python SDK docs and an entry in mkdocs.yml navigation.

Usage of
PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS
were removed in PrefectHQ#8362

There is no reference to them anywhere else, except in settings.py where they are declared. They are just clutter when you open the settings page in the self-hosted server.
@jaraics jaraics requested a review from a team as a code owner April 28, 2024 18:58
Copy link

netlify bot commented Apr 28, 2024

👷 Deploy request for prefect-docs-preview pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 46fb52a

@jaraics jaraics changed the title Clean up unused settings/experimental workpool flags Clean up unused settings/experimental workpool flags PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS Apr 29, 2024
@bunchesofdonald bunchesofdonald self-assigned this Apr 29, 2024
Copy link
Contributor

@bunchesofdonald bunchesofdonald left a comment

Choose a reason for hiding this comment

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

Hi @jaraics, thanks for the contribution! I really appreciate you trying to help keep the codebase clean. I left one note about a change we should make, but otherwise this is great.

src/prefect/settings.py Show resolved Hide resolved
@serinamarie serinamarie added the fix A fix for a bug in an existing feature label Apr 29, 2024
@jaraics
Copy link
Contributor Author

jaraics commented Apr 29, 2024

Hi @bunchesofdonald ! Thanks for taking the time and explaining the reasone. I appreciate that!
I've added them to REMOVED_EXPERIMENTAL_FLAGS. (Also added your explanation as comments to REMOVED_EXPERIMENTAL_FLAGS)

@bunchesofdonald
Copy link
Contributor

Hi @jaraics, it looks like this setting is still used in places with the experimental_parameter decorator. Anywhere that decorator is used with the group of work_pools ultimately ties back to these flags. It's hard to find, I can see why you missed it.

Work pools have been in place for more than a year, I think we can safely remove the decorators if you're up for it.

@jaraics
Copy link
Contributor Author

jaraics commented May 1, 2024

Hi @jaraics, it looks like this setting is still used in places with the experimental_parameter decorator. Anywhere that decorator is used with the group of work_pools ultimately ties back to these flags. It's hard to find, I can see why you missed it.

Work pools have been in place for more than a year, I think we can safely remove the decorators if you're up for it.

Hey @bunchesofdonald I definatelly wish to take this home, I'm just having a hard time marrying a django project with prefect at this time (I wish there were tutorials/better support for it). Give me a couple days and I'll get back on this fix.

Copy link
Contributor

This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment.

@jaraics jaraics changed the title Clean up unused settings/experimental workpool flags PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS Clean up unused settings/experimental workpool flags PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_WARN_WORK_POOLS May 22, 2024
@bunchesofdonald
Copy link
Contributor

@jaraics looks like we're getting close, but there's a few test fails that seem related to this.

@jaraics
Copy link
Contributor Author

jaraics commented May 23, 2024

@jaraics looks like we're getting close, but there's a few test fails that seem related to this.

@bunchesofdonald It seems the tests are for these flags. I think I should remove the tests. Is that acceptable?

@bunchesofdonald
Copy link
Contributor

@bunchesofdonald It seems the tests are for these flags. I think I should remove the tests. Is that acceptable?

I don't think we need to remove the tests here, the only 'true' failure I see is that the work-queue ls cli command is checking for the work-pool experiment. I think we can fix that up by removing this logic branch: https://github.com/PrefectHQ/prefect/blob/main/src/prefect/cli/work_queue.py#L399-L432

The others are our UI settings tests, you should just be able to remove work_pools from the sets in those tests.

Let me know if you run into issues!

@jaraics
Copy link
Contributor Author

jaraics commented May 24, 2024

@bunchesofdonald Ok, I think I fixed the tests. (I can't be sure, they all time out on my PC, maybe I'm missing some setup. A devcontainer would be nice)

@bunchesofdonald
Copy link
Contributor

@bunchesofdonald Ok, I think I fixed the tests. (I can't be sure, they all time out on my PC, maybe I'm missing some setup. A devcontainer would be nice)

The main failure is fixed. I took the liberty of fixing up the ui-settings tests here: 34ae3e9

I'm sorry you're having issues with the tests, it's unusual that they would all time out. The docker suite can timeout locally while building images, but the others tend to be fairly quick. There's not a whole lot of setup to do, I have a virtual env with Python 3.8, and to install the packages I use pip install -e '.[dev]'. One thing you might try is running with a single test runner with pytest -n0 the default is to run one worker per CPU core, but that could be overwhelming your PC.

A dev container would be nice, would you mind writing up an issue?

@bunchesofdonald bunchesofdonald merged commit b9eba01 into PrefectHQ:main May 24, 2024
24 of 25 checks passed
@jaraics jaraics deleted the patch-1 branch May 25, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A fix for a bug in an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants