Fix for Windows #29935
Fix for Windows #29935
Conversation
boring-cyborg
bot
added
the
area:Scheduler
|
@casra-developers - I think havinb half-baked solutions like that is just a starting point. What I am worried about is that it has no support for testing it and that it does not prevent (and will not prevent) anyone from using non-windows compliant behaviours in the future. But maybe this PR might be turned in something more complete. We have #12874 and #10388 opened to make Airflow works for Windows. And Honestly, I do not think we are far from at least being able to run airflow on Windows for local development natively. My idea and suggestion for this PR - why don't we try to make a running set of tests for Windows as part of the a Airflow CI - at least for some basic "airflow" tests. It does not have to be complete set of tests, but just running the "Core" and "Always" tests on a simple "airflow" installation without any extras. This will not use any of the Breeze testing framework - which is more complete and comprehensive, but it woul just make sure that a) you can create airflow virtualenv on windows This all should be possible with the standard way how Github Actions python regular projects are run https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python and we even could have a separate workflow for it. GitHubActions has full support for running the jobs on windows runners, so it does not seem that woudl require a lot of effort. The only real effort will be to fix (or deliberatly skip if we have good reason) all the tests which will be failing. WDYT? |
|
@potiuk I agree with your suggestion and actually can have a look at it. My problem is, that since I have to do this in a professional environment with many other projects, my time is very limited. My experience with Github actions is also very limited. Given these limitations its probably unlikely that I can do this except if I get lucky and figure all this out first try. Is there someone willing to help with this? |
Of course I can help if you have problems - and we can discuss it in this PR - you can just add the workflow here and add workflow to install the environment and run the tests - there. Generally installing python venv, choosing windows machine and running tests with pytest is basically following the tutorials of Github Actions - those are very standard steps. |
There might be some complexity in skipping some tests on windows most likely, but this is just a standard pytest fixture to apply for tests that : |
|
And we do not have to hurry with that, we can iterate for as long as we need . |
|
I have added a new .yml file containing the steps I was able to guess in order for the test procedure. Things I don't now are:
I am sure those things are quite obvious once you know the process, but I tried to research those things with the provided resources, google and the existing actions and am not sure what to do exactly. |
Just
Taking it from the current tests (just looked at the output). That would be good start I think:
Yep. What this sequence does can be repeated - locally create a venv, install airflow and run tests. There is little need to run the whole workflow in such a simple case - my approach is usually to run it locally and once I think it's good to test - push it to PR (and let it do the stuff). There is also https://github.com/nektos/act to simulate the whole GA environment (sort of) but in your case, if you have a windows machine runnig
Yeah. "actions" are overrated. I tend to use them for very standard stuff that you likely already have locally (like checkout the repo or install python) and all the rest shoudl be just commands you can repeat locally. This is it for now no more is needed (just fixing the indentaion in the .yml file - I recommend intellij/vscode support for the github action yaml (they come built-in with github plugins I think) - they will help wiht making the yaml file validated and provide autocompletion. |
|
Thank you so much for these extensive instructions! This really helped me to write the action and test the process locally. One issue that remains though is that the extension I found for vscode (Github Actions v3.0.1) doesn't like my yaml file at all. This is very weird since I followed the online resources provided by Github. Are there some project specific guidelines being enforced here and how can I use the "runs-on: windows-latest" option with the "runs" attribute instead of the "jobs" attribute? |
|
Sadly, I cannot run the tests in a venv since There seems to be a header file missing when building the module. |
|
You need to install MySQL in order to install |
|
So this means that I would have to add these installations as steps in the action I assume. Are they necessary or can we just use the sqlite backend for the tests? |
The indentation is wrong for those lines - they should be indented more - this is simply not valid yaml if it is not. |
|
I think it might be a good idea to get a way of installing airflow devel
without MySQL, but for now it requires it (because for development you
usually need postgres and MySQL to test stimuff with those). For now you
might just install it without 'devel' extra and then we can add individual
extras as they will be needed - that might also help with the task of
adding a simpler devel extra the will cover the basics)
Note - all the extras are explained here
https://airflow.apache.org/docs/apache-airflow/stable/extra-packages-ref.html
pon., 13 mar 2023, 10:00 użytkownik casra-developers <
***@***.***> napisał:
… So this means that I would have to add these installations as steps in the
action I assume. Are they necessary or can we just use the sqlite backend
for the tests?
—
Reply to this email directly, view it on GitHub
<#29935 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAERMIYJWQ7QDVTMEZD5VNLW33O2HANCNFSM6AAAAAAVQ5QORU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I fixed the indentation in the .yml file but the plugin still complains about "jobs" and "on" not being allowed properties and "runs" missing. name: 'Running Tests on Windows'
description: 'Verification that tests run on Windows.'
on: [push]
jobs:
test:
runs-on: windows-latest
steps:
- name: Check out repository code
uses: actions/checkout@v2
- name: Setup Python
uses: actions/setup-python@v2
with:
python-version: "3.x"
- name: Setup venv
run: |
python -m venv venv
.\venv\Scripts\activate
- name: Install dependencies
run: |
pip install ".[devel]"
- name: Run tests
run: |
pytest --verbosity=0 --strict-markers --durations=100 --maxfail=50 --color=yes --junitxml=/files/test_result-Always-sqlite.xml --timeouts-order moi --setup-timeout=60 --execution-timeout=60 --teardown-timeout=60 --output=/files/warnings-Always-sqlite.txt --disable-warnings -rfEX --with-db-init --ignore tests/providers --ignore tests/charts testsI used this site among others to create the file: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python |
I have read through the article but I was not able to find the extras that do not require to install anything for the setup to run properly. Could you give me some directions on what to do to setup the venv properly? |
Just start with |
|
The It seems to me that we are running into the same issues regarding process handling, that this PR wants to address in the Airflow code base. How should we proceed here? Changing the source code in "pytest_timeouts" does not seem to be a good idea. |
This comes from some of the parameters passed in pytest (timeouts related) disabling them (i.e. not using the timeout parameters of pytest]) is the right way to go. |
|
Just running |
Maybe just the plugin that we have being installed causes it. This is the reason why we really need to do the exercise you are doing and fix any kind of hidden dependencies we have. If we do not test it on windows, we will never find out. This is part of the job to be done to make it works fine on Windows. In this case likely adding |
|
I am making a bit of progress in getting the tests to run in venv. One thing I forgot to mention before is that gunicorn is kind of a bad choice for cross-platform support as it is not available on Windows. What we ended up doing for getting the logs from our workers is using basically the same code with waitress instead of gunicorn. It is compatible with all platforms and works almost the same as gunicorn. Should we create an issue for this? |
|
Ok... I got it to at least run the tests, but the issue is that there are just too many instances where things are needed that don't exist on Windows. Be it gunicorn or pytest markers that require timeouts. My current command looks like this: I had to exclude the following packages in setup.py:
The following additional installs where required to still run the tests:
For me there are two options on the table:
We have been using Airflow successfully for two years now with Windows workers, so it can work in a limited sense. It is a very good platform and helped us a great deal with organizing our internal workflows. So it would be awesome if we could find a way to make at least this limited support possible without the need to apply manual fixes to the module after an update. What are your thoughts? |
My thoughts are that indeed we are a bit far from windows support and putting that on you here is too much of an ask :). But at least we know have the issue here and some attempts to show what challenges it brings. (and we can keep it as reference for #12874 and #10388 so that we can always find out the history and attempts when we decide to seriously tackle this. Sorry for putting you through this, but I figured it might be worth trying (and maybe it would turn out it's easier than I thoguht). It is more difficult (after seeing your attempts). If you really just want to have this small "If WINDOWS and they make your life easier, I am happy if you can open a new PR with just those changes and leave that one it it's current state for the future (set it to draft maybe). I hope I have not pushed too much and that we will all learn something useful from that :) |
|
Perfectly understandable. I have the feeling that the main issue is #10388 and if this would work somehow, many other things would also work. The waitress vs. gunicorn thing also is something that could be investigated, since I assume that the replacement should harmonize well with flask. |
|
New PR is created #30249 |
|
I seem to be lacking the permission to convert this PR to a draft: |
|
No - you should be able to do it. But I just did it for you |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
This PR adds checks to make sure that "os" methods that are not supported on Windows are not used on workers. Furthermore, we would appreciate it that methods that are only available on non-Windows systems would be scoped such that they do not raise exceptions on Windows systems. In another PR we have introduce the "airflow.utils.platform.IS_WINDOWS" constant so it should be used.
Thank you in advance