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

Fix windows file limit check #10059

Merged
merged 6 commits into from
Jun 26, 2023

Conversation

rpeden
Copy link
Contributor

@rpeden rpeden commented Jun 24, 2023

Prefect 2.10.17 added a Unix only import in utilities/filesystem.py that breaks when running Prefect server or an agent on Windows.

Credit to @grenzi for suggesting a solution to this; this PR only differs from their PR #10055 in calling getmaxstdio without adding a dependency on pywin32.

This PR calls a function in the Windows Universal C runtime to perform the same check on Windows, and scopes the resource module import to ensure it won't run on Windows. Using ctypes in this instance should be safe, as Microsoft is unlikely to remove the Universal C runtime DLL from Windows given how many of their own applications depend on it.

See https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getmaxstdio for more detail.

Closes #10054
Closes #10058

Although this PR adds a test for get_open_file_limit, many other tests in tests/utilities/test_filesystem.py fail on Windows due to hardcoded Unix file paths. In case it is useful, I opened a draft PR (#10065) with a potential solution that makes the failing tests work cross-platform.

Example

Before: run prefect server start on Windows and get an error: ImportError: No module named 'resource'
After: prefect server start works as expected.

The same applies for the prefect agent start command.

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.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a label categorizing the change e.g. fix, feature, enhancement, docs.

For documentation changes:

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

@netlify
Copy link

netlify bot commented Jun 24, 2023

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit bea666c

@grenzi
Copy link

grenzi commented Jun 24, 2023

@rpeden It's been many a year since i've been in the OS level api calls.

This is great. Avoids a dependency, and uses os.name which is, from my googling the differences here on a sat morning, probably a simpler alternative. (also, today i learned about Jython, which causes os.name to return 'java' - what an odd beast it is)

Anyway, I closed my PR and referenced here since this is a better solution. Thanks!

@rpeden rpeden marked this pull request as ready for review June 25, 2023 17:53
@rpeden rpeden requested a review from a team as a code owner June 25, 2023 17:53
@desertaxle desertaxle added the fix A fix for a bug in an existing feature label Jun 26, 2023
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.

LGTM! Thanks @rpeden!

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.

2.10.17 failure starting prefect agent on a windows machine 2.10.17 will not start on windows host
3 participants