prefect icon indicating copy to clipboard operation
prefect copied to clipboard

Clean up unused settings/experimental workpool flags PREFECT_EXPERIMENTAL_ENABLE_WORK_POOLS and PREFECT_EXPERIMENTAL_WARN_WORK_POOLS

Open jaraics opened this issue 9 months ago • 9 comments

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.

jaraics avatar Apr 28 '24 18:04 jaraics

Deploy request for prefect-docs-preview pending review.

Visit the deploys page to approve it

Name Link
Latest commit 46fb52a1087a9ca4a3b06010e710bf93a0928a54

netlify[bot] avatar Apr 28 '24 18:04 netlify[bot]

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)

jaraics avatar Apr 29 '24 15:04 jaraics

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.

bunchesofdonald avatar Apr 30 '24 14:04 bunchesofdonald

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.

jaraics avatar May 01 '24 07:05 jaraics

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.

github-actions[bot] avatar May 15 '24 08:05 github-actions[bot]

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

bunchesofdonald avatar May 23 '24 18:05 bunchesofdonald

@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?

jaraics avatar May 23 '24 19:05 jaraics

@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!

bunchesofdonald avatar May 23 '24 19:05 bunchesofdonald

@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)

jaraics avatar May 24 '24 09:05 jaraics

@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: https://github.com/PrefectHQ/prefect/pull/13144/commits/34ae3e9b4c3ee905c62fe4fd2ac9ac0920de5258

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 avatar May 24 '24 14:05 bunchesofdonald