You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/12/16 14:10:27 UTC
[GitHub] [airflow] casra-developers opened a new pull request #16110: Added windows extensions
casra-developers opened a new pull request #16110:
URL: https://github.com/apache/airflow/pull/16110
This PR was created after a discussion in this [post](https://github.com/apache/airflow/issues/10388#issuecomment-848867351). @potiuk asked to be mentioned here so he can work with us to integrate those gradual changes.
The aim of this PR is to gradually enable support for the Windows Platform. The changes in this PR allow Airflow to be used with on a Windows System as Dask-Worker. Other things like Web-Server and Task-Scheduling are not possible because of the way processes are handled in Airflow. Next steps would be to find suitable alternatives to those POSIX process management concepts that work on Windows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853599666
I feel we should reimplement it, either by referencing a [publicly available list of signals](https://www.man7.org/linux/man-pages/man7/signal.7.html), or refactor the underlying code to not use those values on Windows at all. Could you list out places that use these signal values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651726925
##########
File path: airflow/utils/process_utils.py
##########
@@ -155,11 +162,13 @@ def execute_interactive(cmd: List[str], **kwargs):
"""
log.info("Executing cmd: %s", " ".join(shlex.quote(c) for c in cmd))
- old_tty = termios.tcgetattr(sys.stdin)
- tty.setraw(sys.stdin.fileno())
+ if not IS_WINDOWS:
+ old_tty = termios.tcgetattr(sys.stdin)
+ tty.setraw(sys.stdin.fileno())
+
+ # open pseudo-terminal to interact with subprocess
+ master_fd, slave_fd = pty.openpty()
Review comment:
IIRC `os.system()` has issues on Windows waiting for the underlying command to finish (the process is executed asynchronously or something like that). Django’s `dbshell` (which is basically the same thing) simply uses `subprocess.run()` so we probably should do the same.
https://github.com/django/django/blob/225d96533a8e05debd402a2bfe566487cc27d95f/django/db/backends/base/client.py#L22
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-943219956
@casra-developers Before making a rebase, it is worthwhile to merge all the changes.
```
git rebase -i HEAD^^^^^^^^^^^
```
And then you need to replace all the `pick` commands with `fixup` commands except the first one. You will have to use `git push --force` when pushing the change.
If you have a new branch that you want to put in this PR then you can just overwrite all commits in that PR.
```shell
git checkout old-branch
git reset --hard new-branch
git push --force
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640503874
##########
File path: airflow/windows_extensions/__init__.py
##########
@@ -0,0 +1 @@
+from airflow.windows_extensions.Signals import Signals
Review comment:
Wait -- we have Signals and signals.
Don't do that. Put them both in signals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849582882
Thank you so much for all the feedback. I will look into it and try address all the various points.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640500018
##########
File path: airflow/models/baseoperator.py
##########
@@ -955,7 +955,7 @@ def __deepcopy__(self, memo):
for k, v in self.__dict__.items():
if k not in shallow_copy:
- setattr(result, k, copy.deepcopy(v, memo)) # noqa
+ setattr(result, k, v if type(v).__name__ == 'module' else copy.deepcopy(v, memo)) # modules cannot be pickled
Review comment:
What module is being copied? We've already got the shallow_copy_attrs list -- so anything not deep-copyable should probably go in that list
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651709940
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
Windows API can handle both forward and backward slashes (except some very old legacy stuff, none of which is used by Python’s path libraries), so it should be fine if we use forward slashes everywhere. For rare cases where some third-party packages rely on legacy APIs, we can call `os.path.normpath` to convert the slashes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r650834156
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -141,7 +143,7 @@ def run_command(self, run_with=None):
universal_newlines=True,
close_fds=True,
env=os.environ.copy(),
- preexec_fn=os.setsid,
+ start_new_session=True
Review comment:
I guess that one we should do regardless form the windows support to avoid race condition.
##########
File path: airflow/utils/timeout.py
##########
@@ -17,21 +17,49 @@
# under the License.
import os
+from threading import Timer
import signal
-
+from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
+
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
def __init__(self, seconds=1, error_message='Timeout'):
super().__init__()
+ self._timer: Timer = None
Review comment:
This is more correct (need also Optional import).
In the past (maybe even still) `mypy` will add Optional automatically if the default is = None but it has been deprecated https://stackoverflow.com/questions/62732402/can-i-omit-optional-if-i-set-default-to-none
```suggestion
self._timer: Optional[Timer] = None
```
##########
File path: setup.cfg
##########
@@ -156,6 +156,9 @@ install_requires =
typing-extensions>=3.7.4;python_version<"3.8"
unicodecsv>=0.14.1
werkzeug~=1.0, >=1.0.1
+ # needed for generating virtual python environments when running tasks
+ virtualenv>=20.4.3
+ psycopg2>=2.8.6
Review comment:
They should both be removed from here. They are already present as extras. You should simply install airflow with postgres and virtualenv extras:
```
pip install "apache-airflow[postgres,virtualenv]" --constraint \
https://raw.githubusercontent.com/apache/airflow/constraints-${AIRFLOW_VERSION}/constraints-${PYTHON_VERSION}.txt
```
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
One more comment here. If we are talking about scheduler running on Linux/Unix and worker running on Windows, we also have problem with directory separator (`/` -> `\`) for dags that are in sub-folders.
I thought we could make airflow to accept the file URI instead in the command line: https://datatracker.ietf.org/doc/html/rfc8089 but it does not support relative paths (officially at least).
So probably the best approach is to make Airlfow replaces `/` to `\` on windows.
##########
File path: airflow/utils/process_utils.py
##########
@@ -155,11 +162,13 @@ def execute_interactive(cmd: List[str], **kwargs):
"""
log.info("Executing cmd: %s", " ".join(shlex.quote(c) for c in cmd))
- old_tty = termios.tcgetattr(sys.stdin)
- tty.setraw(sys.stdin.fileno())
+ if not IS_WINDOWS:
+ old_tty = termios.tcgetattr(sys.stdin)
+ tty.setraw(sys.stdin.fileno())
+
+ # open pseudo-terminal to interact with subprocess
+ master_fd, slave_fd = pty.openpty()
Review comment:
I think this method is needlessly complicated and can be simplified to work same on both win/linux.
We are not interested in the command output here. Simple `os.system(cmd)` should do the job on both Windows and Linux. We just need to change the cmd to be string and format it appropriately using `"` around filenames (sqlite and mysql) and find where the binaries are with `shutil.which`
I tested on linux that this works (for linux but shutil.which is cross-platform and should work on windows too).
```
python -c "import os,shutil; os.system(shutil.which('sqlite3'))"
```
##########
File path: airflow/utils/process_utils.py
##########
@@ -79,7 +86,7 @@ def signal_procs(sig):
else:
raise
- if pgid == os.getpgid(0):
+ if IS_WINDOWS and pgid == os.getpid() or not IS_WINDOWS and pgid == os.getpgid(0):
Review comment:
:D Probably best to:
```
my_pgid = os.getpid() if IS_WINDOWS else os.getpgid(0)
if pgid == my_pgid:
...
```
By the way maybe also `I refuse to kill myself` -> `I refuse to send signal to myself`
##########
File path: airflow/models/baseoperator.py
##########
@@ -955,7 +955,7 @@ def __deepcopy__(self, memo):
for k, v in self.__dict__.items():
if k not in shallow_copy:
- setattr(result, k, copy.deepcopy(v, memo)) # noqa
+ setattr(result, k, v if type(v).__name__ == 'module' else copy.deepcopy(v, memo)) # modules cannot be pickled
Review comment:
Would be great to get clarity here indeed .
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,14 +83,28 @@ def _render_filename(self, ti, try_number):
'ts': ti.execution_date.isoformat(),
'try_number': try_number,
}
- return self.filename_jinja_template.render(**jinja_context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.execution_date.isoformat(),
- try_number=try_number,
- )
+ result_name = self.filename_jinja_template.render(**jinja_context)
+ else:
+ result_name = self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.execution_date.isoformat(),
+ try_number=try_number,
+ )
+
+ # replace ":" with "_" for windows systems
+ if is_windows() and ':' in result_name:
+ print(''.join([
+ 'WARNING: Log file template contains ":" characters ',
+ 'which cannot be used on Windows systems.\n\n',
+ 'Please modify "airflow.cfg":\n',
+ '\t e.g. log_filename_template = {{{{ ti.dag_id }}}}/{{{{ ti.task_id }}}}/{{{{ ts }}}}/{{{{ try_number }}}}.log -> ',
+ 'log_filename_template = {{{{ ti.dag_id }}}}/{{{{ ti.task_id }}}}/{{{{ ts | replace(":", "_") }}}}/{{{{ try_number }}}}.log\n\n',
+ 'The offending ":" have been replaced with "_" characeters but note that this might cause other systems ',
+ 'which are configured differently to not find the log files.'
+ ]))
+ result_name = result_name.replace(':', '_')
+ return result_name
Review comment:
I'd also be for it. The warning is fine to keep like it, but we could simply change default configuration of log_filename_template to contain the replace filter. This will keep backwards compatibility for people who already have airflow but all new installations will use the ones with replace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-945373000
@mik-laj thank you very much for your instructions. I will try to go for the second option as the branch I was trying to rebase is now pretty much trashed beyond repair. I will update this thread as soon as I have tried to integrate our new branch into the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986620429
@uranusjr sorry for the late reply. I was away for three weeks and had no access to my business mail. I will pull the latest changes from the main branch and try to resolve the merge conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986927314
Just let me know if I can assist the process in any capacity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-988790557
Thank you for your answer. We had some weird test interactions ourselves in other projects so its completely understandable. Interesting news about the "Merge Queue", thanks for sharing. I will be on standby and integrate the main branch again once the 4 failing tests are fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-998016026
Awesome! Thanks everyone for the patience and support :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-950653472
Looks like the Quarantined job ran for too long and was killed. There were failures but we don't know what. But those tests are there because they are unsteady in the first place, so perhaps we don't need to worry about those.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640502152
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
+ """
+ Returns true if executing system is Windows
+ """
+ return platform.system() == 'Windows'
+
+def choose_by_platform(posix_option, windows_option):
+ """
+ Returns either POSIX or Windows option based on executing platform
+ :param posix_option: Option for POSIX system (e.g. path to executable)
+ :param posix_option: Option for Windows system (e.g. path to executable)
+ :returns: Choice based on platform
+ """
+ return windows_option if is_windows() else posix_option
Review comment:
New line missing at end of file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-875625692
@casra-developers I've got a WIP stash that fixes that issue, just trying to juggle it, two talks for Airflow Summit and other PR reviews 😁 .
I'll open a PR soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r649426106
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
This is not the right fix for this.
There's a bug in a previous feature where the "dag folder" should be replaced with `DAG_FOLDER` which is then automaticall replaced, but this isn't working.
We should fix that rather than adding a new config and new way of making this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-939552459
Looks like
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-940848751
Sadly not, I had trouble with the rebase, hence my question how to proceed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640502936
##########
File path: airflow/windows_extensions/Signals.py
##########
@@ -0,0 +1,35 @@
+import enum
+
+class Signals(enum.Enum):
Review comment:
Why do need this on Windows? -- it doesn't support most of these signals does it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640501209
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
Review comment:
We've already got `airflow.utils.platform` -- put all this in there instead of creating a new module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-995851638
If it does not help - ask here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-917861965
Yes, the plan is to continue. With that said I don't think we can continue until we have figured out why this crash of the scheduler happens. It did not do that before the rebase. Anyone got an idea?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640668355
##########
File path: airflow/windows_extensions/pty.py
##########
@@ -0,0 +1,170 @@
+"""Pseudo terminal utilities."""
+
+# Bugs: No signal handling. Doesn't set slave termios and window size.
+# Only tested on Linux.
+# See: W. Richard Stevens. 1992. Advanced Programming in the
+# UNIX Environment. Chapter 19.
+# Author: Steen Lumholt -- with additions by Guido.
+
+from select import select
+import os
+from airflow.windows_extensions import tty
+
+__all__ = ["openpty","fork","spawn"]
+
+STDIN_FILENO = 0
+STDOUT_FILENO = 1
+STDERR_FILENO = 2
+
+CHILD = 0
+
+def openpty():
+ """openpty() -> (master_fd, slave_fd)
+ Open a pty master/slave pair, using os.openpty() if possible."""
+
+ try:
+ return os.openpty()
+ except (AttributeError, OSError):
+ pass
+ master_fd, slave_name = _open_terminal()
+ slave_fd = slave_open(slave_name)
+ return master_fd, slave_fd
+
+def master_open():
+ """master_open() -> (master_fd, slave_name)
+ Open a pty master and return the fd, and the filename of the slave end.
+ Deprecated, use openpty() instead."""
+
+ try:
+ master_fd, slave_fd = os.openpty()
+ except (AttributeError, OSError):
+ pass
+ else:
+ slave_name = os.ttyname(slave_fd)
+ os.close(slave_fd)
+ return master_fd, slave_name
+
+ return _open_terminal()
+
+def _open_terminal():
+ """Open pty master and return (master_fd, tty_name)."""
+ for x in 'pqrstuvwxyzPQRST':
+ for y in '0123456789abcdef':
+ pty_name = '/dev/pty' + x + y
+ try:
+ fd = os.open(pty_name, os.O_RDWR)
+ except OSError:
+ continue
+ return (fd, '/dev/tty' + x + y)
+ raise OSError('out of pty devices')
Review comment:
This is a very good question. I would have assumed as much since this file comes with every python installation on Windows, but then again the reason why we pasted it in here was that the tty module did not work properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651467469
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -141,7 +143,7 @@ def run_command(self, run_with=None):
universal_newlines=True,
close_fds=True,
env=os.environ.copy(),
- preexec_fn=os.setsid,
+ start_new_session=True
Review comment:
Has been added again. Tests on a Windows-System show that this works fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-874520917
Any news on #8061. As I understand it this is currently the only thing holding back the PR. Anything I can help with from our side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-891119228
Very nice that this Dask-Path issue got resolved! @potiuk I have rebased our forked master on my dev machine and tried to test it. I setup venvs on our Windows-Dask-Worker and our Ubuntu Airflow-Webserver / Scheduler and installed the package from a wheel which I've built via 'python -m sdist bdist_wheel'. Installation and DB Schema upgrades where smooth, but when I log into the web-interface I get a lot of errors 404.
on the airflow server:
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/appbuilder/css/bootstrap.min.css.map HTTP/1.1" 404 527 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:34 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:34 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:34 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:35 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:35 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:36 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
in the browser console:
For me it looks like there are some icons or other resources that cannot be located. Clicking is impossible in this UI, I have to type the dag_id into the URL to navigate but starting is not possible.
Can you reproduce this behavior with the version currently on main or is there something wrong with our test-environment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-874520917
Any news on #8061? As I understand it this is currently the only thing holding back the PR. Anything I can help with from our side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Anurag-Shetty commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
Anurag-Shetty commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r719106626
##########
File path: airflow/utils/configuration.py
##########
@@ -21,6 +21,7 @@
from tempfile import mkstemp
from airflow.configuration import conf
+from airflow.platform import IS_WINDOWS
Review comment:
should it be "from airflow.utils.platform import IS_WINDOWS" ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-934400461
Today I finally had a few hours to spare and look into it again. The hint about the cloudpickle and distributed versions was the solution to get the airflow version from the main-branch working, thanks @Anurag-Shetty :)
Sadly, I was not able to run the git rebase without issues, so I ended up cloning the main-branch and implementing the necessary changes locally from our forked repository.
As is, the changes required to use a Windows based Dask-Worker are actually quite minimal:
- airflow/utils/platform.py: Added the IS_WINDOWS constant
- airflow/utils/process_utils.py: Made some imports conditional
- airflow/utils/timeout.py: Added second thread-based rimwour-implementation for Windows systems
- airflow/utils/configuration.py: Added check to not use the os.fchmod function on Windows
- airflow/task/task_runner/base_task_runner.py: Made imports conditional
Hacks for running the dags from the another path are not necessary anymore. Logs also do not need to be copied anymore but can be served via the **python -m http.server** command which can be run as a service on the Dask-Worker.
The question now is how to integrate those few changes into the main-branch. Should I just overwrite the entire code within the original branch which was reviewed within this thread? Should I create a new pull-request from the newly cloned repository?
@potiuk I assume you are quite familiar with merging procedures on GitHub so I would do what you think is best.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651460996
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
Thanks for explaining. I was not familiar with mypy but this seems very useful also for future projects :)
I have commited the change request and there is now an Optional annotation.
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
Thanks for explaining. I was not familiar with mypy but this seems very useful also for future projects :)
I have comited the change request and there is now an Optional annotation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-963468298
Ok I did another rebase and tested a local build on our test-environment. I had some issues with the pre-commit hooks since some bash scripts had \r\n line endings instead of \n (might be VSCode running on Windows being the culprit here). @potiuk happy to hear that we can soon close the PR and I hope I do not have to do another rebase. We as a company use primarily Windows systems which makes development a bit difficult in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853612966
I try to narrow it down. Luckily, it seems to be mostly two files that require those dependencies in a limited manner:
- airflow/utils/process_utils.py
- **termios**: On line 159 in execute_interactive
- **termios**: On line 180 in execute_interactive
- **tty**: On line 160 in execute_interactive
- **pty**: On line 163 in execute_interactive
- airflow/utils/timeout.py
- **signal**: On line 40 in __enter__
- **signal**: On line 41in __enter__
- **signal**: On line 47 in __exit__
In "process_utils.py" we probably can just use IS_WINDOWS to not use the missing features. The timeout one is probably a bit more tricky although we could still use the thread-based Windows implementation without hiding it behind the modified signal module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-891119228
Very nice that this Dask-Path issue got resolved! @potiuk I have rebased our forked master on my dev machine and tried to test it. I setup venvs on our Windows-Dask-Worker and our Ubuntu Airflow-Webserver / Scheduler and installed the package from a wheel which I've built via 'python -m sdist bdist_wheel'. Installation and DB Schema upgrades where smooth, but when I logged into the web-interface I get a lot of errors 404.
on the airflow server:
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/appbuilder/css/bootstrap.min.css.map HTTP/1.1" 404 527 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:33 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:34 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:34 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:34 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:35 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:35 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
[IP_ADDRESS] - - [02/Aug/2021:17:24:36 +0200] "GET /static/ HTTP/1.1" 404 527 "https://vsrv-afdev02/home" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36"
in the browser console:
For me it looks like there are some icons or other resources that cannot be located. Clicking is impossible in this UI, I have to type the dag_id into the URL to navigate but starting is not possible.
Can you reproduce this behavior with the version currently on main or is there something wrong with our test-environment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-966878135
I will be away starting Monday for three weeks. If there are any issues which need to be resolved before the changes can be merged I would need to know today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-997043446
Seems like the pwname not used is the last problem @casra-developers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-997477226
Added a fixup to fix the static-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-997682608
@potiuk thank you very much for adding the #noqa in base_task_runner.py. I've rebased the branch 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-949353023
@uranusjr yes you are right (probably forgot to save when resolving the conflicts). I was able to resolve this and the check ```./breeze static-check isort``` now is marked as "passed". The problem now seems to be on the build-pipeline with dangling symlinks as far as I can judge.
```
Run ./scripts/ci/images/ci_prepare_ci_image_on_ci.sh
Skip mounting host volumes to Docker
Good version of docker 20.10.7.
Running 'ci_prepare_ci_image_on_ci.sh'
Using pulled cache strategy for the build.
chmod: cannot operate on dangling symlink 'provider_packages/.flake8'
chmod: cannot operate on dangling symlink 'provider_packages/dist'
chmod: cannot operate on dangling symlink 'provider_packages/pyproject.toml'
ERROR: The previous step completed with error. Please take a look at output above
###########################################################################################
EXITING WITH STATUS CODE 123
###########################################################################################
Finished the script ci_prepare_ci_image_on_ci.sh
Elapsed time spent in the script: 0 seconds
Exit code 123
Error: Process completed with exit code 123.
```
I have no idea if this has to do with the changes I have made but this would be quite odd since I did only change the few Python files, not touching anything else. Is there something I can do to resolve this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853622165
Ok, I will try to implement the changes and do another commit. I will likely take a bit of times since this requires me to setup a Dask-Worker for testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-933155743
As soon as there is a time window I will try to test get the branch working with the points mentioned in the thread you posted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r721052033
##########
File path: airflow/utils/configuration.py
##########
@@ -21,6 +21,7 @@
from tempfile import mkstemp
from airflow.configuration import conf
+from airflow.platform import IS_WINDOWS
Review comment:
True, thanks for pointing this out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-946785448
I have noticed that there is one test failing. Is there something that needs to be changed for the merge to commence?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-995851375
Sure. Yeah. I understand the time constrints. If in doubt - just rebase :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-998036057
Likewise!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-997884907
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986815237
Ah. Cool we are also working on Breeze rewrite in Python, that would make it more "open" to Windows (no WSL2) users. https://github.com/apache/airflow/pull/19992 so this is handy to get this one in for the "local virtualenv" experience as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-907070123
Sorry for taking so long with the testing, I had to work on many other projects where the due dates are approaching therefore I had to postpone the testing of the re-based branch. Also since we have no experience with yarn/python web backends etc. it took some time to figure out how to build the web-assets, but webserver, scheduler and worker all are able to communicate once more.
When testing a very simple dag, the ariflow scheduler crashes with the following traceback:
```
distributed.protocol.pickle - INFO - Failed to serialize <function DaskExecutor.execute_async.<locals>.airflow_run at 0x7f0ba17de8c0>. Exception: Cell is empty
--- Logging error ---
Traceback (most recent call last):
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/worker.py", line 3525, in dumps_function
result = cache_dumps[func]
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/utils.py", line 1497, in __getitem__
value = super().__getitem__(key)
File "/usr/lib/python3.7/collections/__init__.py", line 1027, in __getitem__
raise KeyError(key)
KeyError: <function DaskExecutor.execute_async.<locals>.airflow_run at 0x7f0ba17de8c0>
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/protocol/pickle.py", line 49, in dumps
result = pickle.dumps(x, **dump_kwargs)
AttributeError: Can't pickle local object 'DaskExecutor.execute_async.<locals>.airflow_run'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/scheduler_job.py", line 652, in _execute
self._run_scheduler_loop()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/scheduler_job.py", line 735, in _run_scheduler_loop
self.executor.heartbeat()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/executors/base_executor.py", line 158, in heartbeat
self.trigger_tasks(open_slots)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/executors/base_executor.py", line 188, in trigger_tasks
self.execute_async(key=key, command=command, queue=queue, executor_config=ti.executor_config)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/executors/dask_executor.py", line 84, in execute_async
future = self.client.submit(airflow_run, pure=False)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/client.py", line 1596, in submit
actors=actor,
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/client.py", line 2554, in _graph_to_futures
dsk = dsk.__dask_distributed_pack__(self, keyset)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/dask/highlevelgraph.py", line 959, in __dask_distributed_pack__
client_keys,
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/dask/highlevelgraph.py", line 392, in __dask_distributed_pack__
dsk = toolz.valmap(dumps_task, dsk)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/toolz/dicttoolz.py", line 83, in valmap
rv.update(zip(d.keys(), map(func, d.values())))
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/worker.py", line 3563, in dumps_task
return {"function": dumps_function(task[0]), "args": warn_dumps(task[1:])}
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/worker.py", line 3527, in dumps_function
result = pickle.dumps(func, protocol=4)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/protocol/pickle.py", line 60, in dumps
result = cloudpickle.dumps(x, **dump_kwargs)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 101, in dumps
cp.dump(obj)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 540, in dump
return Pickler.dump(self, obj)
File "/usr/lib/python3.7/pickle.py", line 437, in dump
self.save(obj)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 722, in save_function
*self._dynamic_function_reduce(obj), obj=obj
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 659, in _save_reduce_pickle5
dictitems=dictitems, obj=obj
File "/usr/lib/python3.7/pickle.py", line 638, in save_reduce
save(args)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/usr/lib/python3.7/pickle.py", line 789, in save_tuple
save(element)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/usr/lib/python3.7/pickle.py", line 774, in save_tuple
save(element)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/dill/_dill.py", line 1226, in save_cell
f = obj.cell_contents
ValueError: Cell is empty
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/lib/python3.7/logging/__init__.py", line 1025, in emit
msg = self.format(record)
File "/usr/lib/python3.7/logging/__init__.py", line 869, in format
return fmt.format(record)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/utils/log/colored_log.py", line 95, in format
record = self._color_record_traceback(record)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/utils/log/colored_log.py", line 86, in _color_record_traceback
self.color(self.log_colors, record.levelname) + record.exc_text + escape_codes['reset']
AttributeError: 'CustomTTYColoredFormatter' object has no attribute 'color'
Call stack:
File "/home/airflow/test_environments/venv/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/__main__.py", line 41, in main
args.func(args)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/cli/cli_parser.py", line 48, in command
return func(*args, **kwargs)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/utils/cli.py", line 91, in wrapper
return f(*args, **kwargs)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/cli/commands/scheduler_command.py", line 70, in scheduler
job.run()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/base_job.py", line 245, in run
self._execute()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/scheduler_job.py", line 668, in _execute
self.log.exception("Exception when executing SchedulerJob._run_scheduler_loop")
Message: 'Exception when executing SchedulerJob._run_scheduler_loop'
Arguments: ()
[2021-08-27 11:27:25,761] {process_utils.py:108} INFO - Sending Signals.SIGTERM to GPID 93904
[2021-08-27 11:27:25,894] {process_utils.py:70} INFO - Process psutil.Process(pid=93904, status='terminated', exitcode=0, started='11:27:24') (93904) terminated with exit code 0
[2021-08-27 11:27:25,895] {scheduler_job.py:679} INFO - Exited execute loop
Traceback (most recent call last):
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/worker.py", line 3525, in dumps_function
result = cache_dumps[func]
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/utils.py", line 1497, in __getitem__
value = super().__getitem__(key)
File "/usr/lib/python3.7/collections/__init__.py", line 1027, in __getitem__
raise KeyError(key)
KeyError: <function DaskExecutor.execute_async.<locals>.airflow_run at 0x7f0ba17de8c0>
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/protocol/pickle.py", line 49, in dumps
result = pickle.dumps(x, **dump_kwargs)
AttributeError: Can't pickle local object 'DaskExecutor.execute_async.<locals>.airflow_run'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/airflow/test_environments/venv/bin/airflow", line 8, in <module>
sys.exit(main())
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/__main__.py", line 41, in main
args.func(args)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/cli/cli_parser.py", line 48, in command
return func(*args, **kwargs)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/utils/cli.py", line 91, in wrapper
return f(*args, **kwargs)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/cli/commands/scheduler_command.py", line 70, in scheduler
job.run()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/base_job.py", line 245, in run
self._execute()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/scheduler_job.py", line 652, in _execute
self._run_scheduler_loop()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/jobs/scheduler_job.py", line 735, in _run_scheduler_loop
self.executor.heartbeat()
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/executors/base_executor.py", line 158, in heartbeat
self.trigger_tasks(open_slots)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/executors/base_executor.py", line 188, in trigger_tasks
self.execute_async(key=key, command=command, queue=queue, executor_config=ti.executor_config)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/airflow/executors/dask_executor.py", line 84, in execute_async
future = self.client.submit(airflow_run, pure=False)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/client.py", line 1596, in submit
actors=actor,
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/client.py", line 2554, in _graph_to_futures
dsk = dsk.__dask_distributed_pack__(self, keyset)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/dask/highlevelgraph.py", line 959, in __dask_distributed_pack__
client_keys,
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/dask/highlevelgraph.py", line 392, in __dask_distributed_pack__
dsk = toolz.valmap(dumps_task, dsk)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/toolz/dicttoolz.py", line 83, in valmap
rv.update(zip(d.keys(), map(func, d.values())))
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/worker.py", line 3563, in dumps_task
return {"function": dumps_function(task[0]), "args": warn_dumps(task[1:])}
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/worker.py", line 3527, in dumps_function
result = pickle.dumps(func, protocol=4)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/distributed/protocol/pickle.py", line 60, in dumps
result = cloudpickle.dumps(x, **dump_kwargs)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 101, in dumps
cp.dump(obj)
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 540, in dump
return Pickler.dump(self, obj)
File "/usr/lib/python3.7/pickle.py", line 437, in dump
self.save(obj)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 722, in save_function
*self._dynamic_function_reduce(obj), obj=obj
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/cloudpickle/cloudpickle_fast.py", line 659, in _save_reduce_pickle5
dictitems=dictitems, obj=obj
File "/usr/lib/python3.7/pickle.py", line 638, in save_reduce
save(args)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/usr/lib/python3.7/pickle.py", line 789, in save_tuple
save(element)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/usr/lib/python3.7/pickle.py", line 774, in save_tuple
save(element)
File "/usr/lib/python3.7/pickle.py", line 504, in save
f(self, obj) # Call unbound method with explicit self
File "/home/airflow/test_environments/venv/lib/python3.7/site-packages/dill/_dill.py", line 1226, in save_cell
f = obj.cell_contents
ValueError: Cell is empty
```
It seems to me that there is something wrong in the "_run_scheduler_loop" function in scheduler_job.py. Maybe there are inconsistencies in the data-models eventhough I've ran ```airflow db upgrade``` without any errors.
I hope this helps to resolve the hopefully trivial issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640503132
##########
File path: airflow/windows_extensions/__init__.py
##########
@@ -0,0 +1 @@
+from airflow.windows_extensions.Signals import Signals
Review comment:
```suggestion
from airflow.windows_extensions.signals import Signals
```
Module names should be lower case by convention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-876418370
@ashb thank you for working on it. No need to rush :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-858296974
@jhtimmins Thanks for asking. I tried to address all the points brought up in this discussion, therefore I would suggest that the authors of those comments quickly check whether they consider the solutions to be acceptable. After that we can probably merge except if there is another thing that needs to be addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640501209
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
Review comment:
We've already got `airflow.utils.platform` -- put this function in there instead of creating a new module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r649381846
##########
File path: airflow/utils/process_utils.py
##########
@@ -79,7 +86,7 @@ def signal_procs(sig):
else:
raise
- if pgid == os.getpgid(0):
+ if IS_WINDOWS and pgid == os.getpid() or not IS_WINDOWS and pgid == os.getpgid(0):
Review comment:
I think this needs some parentheses? At least for readability.
##########
File path: setup.cfg
##########
@@ -156,6 +156,9 @@ install_requires =
typing-extensions>=3.7.4;python_version<"3.8"
unicodecsv>=0.14.1
werkzeug~=1.0, >=1.0.1
+ # needed for generating virtual python environments when running tasks
+ virtualenv>=20.4.3
+ psycopg2>=2.8.6
Review comment:
Why is psycopg needed?
##########
File path: airflow/utils/process_utils.py
##########
@@ -155,11 +162,13 @@ def execute_interactive(cmd: List[str], **kwargs):
"""
log.info("Executing cmd: %s", " ".join(shlex.quote(c) for c in cmd))
- old_tty = termios.tcgetattr(sys.stdin)
- tty.setraw(sys.stdin.fileno())
+ if not IS_WINDOWS:
+ old_tty = termios.tcgetattr(sys.stdin)
+ tty.setraw(sys.stdin.fileno())
+
+ # open pseudo-terminal to interact with subprocess
+ master_fd, slave_fd = pty.openpty()
Review comment:
I don’t think the function works after the patch (at least `slave_fd` would become undefined). Since this function is only used to run database commands, maybe we should mark this function as POSIX-only, and implement a separate function for Windows. (Would it be enough to simply brige stdin/out/err to the subprocess?)
##########
File path: airflow/utils/timeout.py
##########
@@ -31,20 +34,34 @@ def __init__(self, seconds=1, error_message='Timeout'):
self.seconds = seconds
self.error_message = error_message + ', PID: ' + str(os.getpid())
- def handle_timeout(self, signum, frame): # pylint: disable=unused-argument
+ def handle_timeout(self, *args): # pylint: disable=unused-argument
"""Logs information and raises AirflowTaskTimeout."""
self.log.error("Process timed out, PID: %s", str(os.getpid()))
raise AirflowTaskTimeout(self.error_message)
def __enter__(self):
try:
- signal.signal(signal.SIGALRM, self.handle_timeout)
- signal.setitimer(signal.ITIMER_REAL, self.seconds)
+ if IS_WINDOWS:
+ if hasattr(self, TIMER_THREAD_ATTR) and getattr(self, TIMER_THREAD_ATTR) is not None:
+ getattr(self, TIMER_THREAD_ATTR).cancel()
+ timer = Timer(self.seconds, self.handle_timeout)
+ setattr(self, TIMER_THREAD_ATTR, timer)
+ timer.start()
Review comment:
The exception message below (about hte current context) doesn’t apply to the thread-based implementation, so we should move the `if IS_WINDOWS:` block out of `try:`.
##########
File path: airflow/utils/timeout.py
##########
@@ -31,20 +34,34 @@ def __init__(self, seconds=1, error_message='Timeout'):
self.seconds = seconds
self.error_message = error_message + ', PID: ' + str(os.getpid())
- def handle_timeout(self, signum, frame): # pylint: disable=unused-argument
+ def handle_timeout(self, *args): # pylint: disable=unused-argument
"""Logs information and raises AirflowTaskTimeout."""
self.log.error("Process timed out, PID: %s", str(os.getpid()))
raise AirflowTaskTimeout(self.error_message)
def __enter__(self):
try:
- signal.signal(signal.SIGALRM, self.handle_timeout)
- signal.setitimer(signal.ITIMER_REAL, self.seconds)
+ if IS_WINDOWS:
+ if hasattr(self, TIMER_THREAD_ATTR) and getattr(self, TIMER_THREAD_ATTR) is not None:
+ getattr(self, TIMER_THREAD_ATTR).cancel()
+ timer = Timer(self.seconds, self.handle_timeout)
+ setattr(self, TIMER_THREAD_ATTR, timer)
+ timer.start()
Review comment:
BTW why do we need to use `has|get|set_attr` here? Why not put them on the instance instead, they would just be unused on POSIX. Or we can do something like
```python
_timeout = ContextManager[None]
class _timeout_windows(_timeout):
... # Implementation for Windows.
class _timeout_posix(_timeout):
... # Implementation for POSIX.
if IS_WINDOWS:
timeout: Type[_timeout] = _timeout_windows
else:
timeout = _timeout_posix
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640688180
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
This is probably not an ideal solution and I forgot to put this in from our repository in the first commit. On Windows the Dask-Worker tries to get the DAG at the location where it is stored on the Airflow server (e.g. /home/airflow/dags). So here we actually force it to use the DAG path in the config file... I don't know how this is handled on a setup involving only Linux machines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r650763917
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
When youi have `self._timer: Timer`, the right hand side of the assignment must evaluate to a `Timer`, which `None` is not. To make Mypy accept this, you need to annotate the type as `Optional[Timer]` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-861199983
I think the biggest thing remaining is the workaround for the Executor bug. Since I am not quite sure where the problem is exactly I don't know how relevant this comment is.
I saw some remarks about the path separators on Windows being different from POSIX. In my experience, when using Python, paths can be written usually either way on Windows e.g.
```
# works
Popen('c:/Windows/System32/notepad.exe')
# also works
Popen('c:\\Windows\\System32\\notepad.exe')
```
To avoid any issues I usually tend to use os.path.join(...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640503514
##########
File path: airflow/windows_extensions/pty.py
##########
@@ -0,0 +1,170 @@
+"""Pseudo terminal utilities."""
+
+# Bugs: No signal handling. Doesn't set slave termios and window size.
+# Only tested on Linux.
+# See: W. Richard Stevens. 1992. Advanced Programming in the
+# UNIX Environment. Chapter 19.
+# Author: Steen Lumholt -- with additions by Guido.
Review comment:
Where did this file come from? What is it licensed under?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-961840377
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-968620717
CI failures are caused by timeouts. There’s also a merge conflict so I’ll try resolving this and re-trigger CI afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640670953
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -86,6 +87,10 @@ def __init__(self, local_task_job):
# pylint: disable=consider-using-with
self._error_file = NamedTemporaryFile(delete=True)
+
+ # HOTFIX: When reporting exceptions, this file was usually locked because it was still opened by this process
+ self._error_file.close()
Review comment:
On Windows we basically just create an empty text file. Not closing it immediately raised an exception while logging since the file was opened by another python process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986859488
> I have noticed that there are four tests of type www which are failing. I'm assuming that this is something that has nothing todo with our changes, right?
I also see it in my PR which I pushed recently, so most likely some bad merge indeed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #16110:
URL: https://github.com/apache/airflow/pull/16110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-995617070
@potiuk thank you for your very helpful advice. In my case it is less about frustration but more about the time I am able to spend on this topic. Since I am part of an SD Dev team at CASRA, these are company resources which are invested. The company has decided that it wants to support Open Source projects such as Airflow but obviously we have to commit to a budget.
With that said, it is true that the rebasing gets less painful every time I do it and therefore it is now something that I can do quite comfortably besides other tasks (including the testing on our Airflow-Linux-Windows test environment).
Something which I probably should have asked earlier is if it is expected of me to investigate those 4 failing checks or if this is something that will be fixed by just rebasing to the current main branch? To be honest, I looked at the checks but I cannot really see how they would relate to my changes therefore I did not feel responsible to investigate further.
Please let me know if I can assist in any way. Airflow is an awesome platform and I am happy to help within the scope available to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r650787593
##########
File path: setup.cfg
##########
@@ -156,6 +156,9 @@ install_requires =
typing-extensions>=3.7.4;python_version<"3.8"
unicodecsv>=0.14.1
werkzeug~=1.0, >=1.0.1
+ # needed for generating virtual python environments when running tasks
+ virtualenv>=20.4.3
+ psycopg2>=2.8.6
Review comment:
It depends on why th module is needed. But I’m quite sure adding this as a harde dependency to Airflow core is the wrong solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-945395012
Good news :)
I've actually went along and just did a hard reset on the master branch of the forked repository to be in sync with the main branch of the airlfow repository. After that I just integrated the changes necessary to run the Dask-Worker on Windows and tested it again on our testing environment.
This means that we should now be ready to merge the master into the main branch.
Please let me know if there are any issues that need to be addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r650820411
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
Yep. Also see https://github.com/apache/airflow/discussions/16423 - might be the right time to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-859276443
@ashb since there is this remove DAG execution issue I assume that it would be wise to remove our workaround and wait until #8061 is resolved. I will remove the workaround as soon as the fix is available and I can merge it into this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849615846
The module *name* should be fine, I think. I was actually referring to the code inside it; Apache has some pretty strict rules on what code can be used in a project, and that module seems like copied from other projects which would be problematic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640683759
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -86,6 +87,10 @@ def __init__(self, local_task_job):
# pylint: disable=consider-using-with
self._error_file = NamedTemporaryFile(delete=True)
+
+ # HOTFIX: When reporting exceptions, this file was usually locked because it was still opened by this process
+ self._error_file.close()
Review comment:
Ok nevermind... I removed the line and it still works on our Dask-Worker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-856494189
I finally was able to test the changes on one of our Windows-Dask-Workers. I removed the "windows_extensions" moldule and instead added checks at the few places where the offending POSIX methods where used. As Dask-Worker this seems to be fine, but I assume that many other use-cases will not work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r649426106
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
This is not the right fix for this.
There's a bug in a previous feature where the "dag folder" should be replaced with `DAG_FOLDER` which is then automaticall replaced, but this isn't working.
We should fix that rather than adding a new config and new way of making this work.
See https://github.com/apache/airflow/blob/9ba796ef40fe833aba58f5aa13a63587106d8ffd/airflow/utils/cli.py#L160-L167 for where the code is (the problem is on the command we send to the executor.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640505652
##########
File path: airflow/windows_extensions/pty.py
##########
@@ -0,0 +1,170 @@
+"""Pseudo terminal utilities."""
+
+# Bugs: No signal handling. Doesn't set slave termios and window size.
+# Only tested on Linux.
+# See: W. Richard Stevens. 1992. Advanced Programming in the
+# UNIX Environment. Chapter 19.
+# Author: Steen Lumholt -- with additions by Guido.
+
+from select import select
+import os
+from airflow.windows_extensions import tty
+
+__all__ = ["openpty","fork","spawn"]
+
+STDIN_FILENO = 0
+STDOUT_FILENO = 1
+STDERR_FILENO = 2
+
+CHILD = 0
+
+def openpty():
+ """openpty() -> (master_fd, slave_fd)
+ Open a pty master/slave pair, using os.openpty() if possible."""
+
+ try:
+ return os.openpty()
+ except (AttributeError, OSError):
+ pass
+ master_fd, slave_name = _open_terminal()
+ slave_fd = slave_open(slave_name)
+ return master_fd, slave_fd
+
+def master_open():
+ """master_open() -> (master_fd, slave_name)
+ Open a pty master and return the fd, and the filename of the slave end.
+ Deprecated, use openpty() instead."""
+
+ try:
+ master_fd, slave_fd = os.openpty()
+ except (AttributeError, OSError):
+ pass
+ else:
+ slave_name = os.ttyname(slave_fd)
+ os.close(slave_fd)
+ return master_fd, slave_name
+
+ return _open_terminal()
+
+def _open_terminal():
+ """Open pty master and return (master_fd, tty_name)."""
+ for x in 'pqrstuvwxyzPQRST':
+ for y in '0123456789abcdef':
+ pty_name = '/dev/pty' + x + y
+ try:
+ fd = os.open(pty_name, os.O_RDWR)
+ except OSError:
+ continue
+ return (fd, '/dev/tty' + x + y)
+ raise OSError('out of pty devices')
Review comment:
Is this going to work on windows?!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-858803535
@ashb can you take a look to see if your requested changes have been addressed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849617108
> The module _name_ should be fine, I think. I was actually referring to the code inside it; Apache has some pretty strict rules on what code can be used in a project, and that module seems like copied from other projects which would be problematic.
So long as it is appropriately licensed, and you can attribute it then it's fine, and we'll put a section in the `NOTICE` file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651910209
##########
File path: airflow/utils/process_utils.py
##########
@@ -155,11 +162,13 @@ def execute_interactive(cmd: List[str], **kwargs):
"""
log.info("Executing cmd: %s", " ".join(shlex.quote(c) for c in cmd))
- old_tty = termios.tcgetattr(sys.stdin)
- tty.setraw(sys.stdin.fileno())
+ if not IS_WINDOWS:
+ old_tty = termios.tcgetattr(sys.stdin)
+ tty.setraw(sys.stdin.fileno())
+
+ # open pseudo-terminal to interact with subprocess
+ master_fd, slave_fd = pty.openpty()
Review comment:
Fine for me too. As long as we don't try to pipe things out which is the reason for the complexity of the original solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-994984675
I feel your frustration in the question. So let me try to be helpful and explain a bit.
I'd advise you to continue rebasing and ping us when you see tests succed. There are conflicts now as you can see. And Ping everyone here when it succeds. In a way when you are a PR "author" it's part of the author's "job" to drag attention of others when you see your PR is ready to merge (green or you only see unrelated issues). This is the best you can do.
It becomes quite obvious if you try to put yourself in our shoes. For you this is only (or maybe one of a few) PRs that you care about. Yet we - as committers had 340 (!) PR merged overt the course of 1 month https://github.com/apache/airflow/pulse/monthly) . There are just a handful of commiters and this is ~20 PRs merged per working day. Some of them taking 20-30 comments on. As you can possibly imagine none of us is on a lookou to merge PRs at the moment they succeed. We have likely 100-200 completeed PR jobs a day. This is also a reason why sometimes main is broken as things slip through.
So it's simply much easier for you to pay an attention and ping use when you see things are "good".
I understand you would like to work in "fire and forget" mode, but simply this is difficult (but possibly merge queue feature which I wrote about earlier, will help with that).
Just as a general note, there is also the old saying In fact if something is more painful, do it more often. if you rebase more often, it pains less overall becaue a) you do it in smaller chunks, b) you learn how to do it fast. I've learned how to rebase my commits and resolve conflicts quickly during working on Airlfow.
So, apologies if it takes longer than you thought and that you have to do it several times, but this is how it works and best you can do is to "vet" your PR and be a little annoying (but not after some time - immediately when you see it is ready to merge).
I hope that is helpful and provides you the right context on why things are like that . Thanks for your understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-994332775
I just quickly want to ask what are the next steps before we merge. Should I wait until the tests are fixed or just rebase and commit again. Thanks very much in advance for directions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-996909103
> Did another rebase +1
I told you it gets the easier, the more often you do it ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-949381469
It's indeed strange; those files are actually listed as changed here, but I'm not able to tell what exactly changed from the diff (probably some invisible stuff). Maybe try `git checkout main -- provider_packages/.flake8 [... other files]` to revert them to latest main and see if that fixes things?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-890582277
Close to merge #16860 :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-859276443
@ashb since there is this remote DAG execution issue I assume that it would be wise to remove our workaround and wait until #8061 is resolved. I will remove the workaround as soon as the fix is available and I can merge it into this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-917674049
Hey @casra-developers - are you going to continue with this? I think it would be pretty useful!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r649849547
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
+ self.seconds = seconds
+ self.error_message = error_message + ': Operation timed out.'
+
+ def handle_timeout(self, *args): # pylint: disable=unused-argument
+ """Logs information and raises AirflowTaskTimeout."""
+ self.log.error("Operation timed out.")
+ raise AirflowTaskTimeout(self.error_message)
+
+ def __enter__(self):
+ try:
+ if self._timer:
+ self._timer.cancel()
+ self._timer = Timer(self.seconds, self.handle_timeout)
+ self._timer.start()
+ except ValueError:
Review comment:
Does this ever happen?
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
+ self.seconds = seconds
+ self.error_message = error_message + ': Operation timed out.'
Review comment:
Why does this not show the PID like the POSIX version?
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
I don’t think mypy would allow this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651726925
##########
File path: airflow/utils/process_utils.py
##########
@@ -155,11 +162,13 @@ def execute_interactive(cmd: List[str], **kwargs):
"""
log.info("Executing cmd: %s", " ".join(shlex.quote(c) for c in cmd))
- old_tty = termios.tcgetattr(sys.stdin)
- tty.setraw(sys.stdin.fileno())
+ if not IS_WINDOWS:
+ old_tty = termios.tcgetattr(sys.stdin)
+ tty.setraw(sys.stdin.fileno())
+
+ # open pseudo-terminal to interact with subprocess
+ master_fd, slave_fd = pty.openpty()
Review comment:
IIRC `os.system()` has issues on Windows waiting for the underlying command to finish (the process is executed asynchronously or something like that). Django’s `dbshell` (which basically does the same thing as `airflow db shell`) simply uses `subprocess.run()` so we probably should do the same.
https://github.com/django/django/blob/225d96533a8e05debd402a2bfe566487cc27d95f/django/db/backends/base/client.py#L22
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-890582360
Would you rebase please to latest `main` @casra-developers ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-874520917
Any news on #8061. As I understand it this is currently the only thing holding back the PR. Anything I can help with from our side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-874520917
Any news on #8061? As I understand it this is currently the only thing holding back the PR. Anything I can help with from our side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640677988
##########
File path: airflow/utils/process_utils.py
##########
@@ -20,14 +20,20 @@
import errno
import logging
import os
-import pty
import select
import shlex
import signal
import subprocess
import sys
-import termios
-import tty
+from airflow.utils.platform_utils import is_windows
+
+if is_windows():
+ from airflow.windows_extensions import termios, tty, pty
+else:
+ import pty
+ import termios
+ import tty
Review comment:
I will do that. If I understand you correctly you basically do import distinction in one file and than just import from there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849476930
@potiuk please let me now how I can assist you (sadly not during the weekend since I have to book my hours ^^). I am aware that the changes so far are more of workarounds than actual platform support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849483475
Thanks @casra-developers . We can work on asynchronous fashion all right here. And I believe adding full windows support to get Airflow up and running on Windows will take quite some time (if it will be there at all). For now what we can do we can add partial support:
1) for development purpose
2) to run particular parts of Airlfow - mainly workers - on Windows, to run particular windows-only tasks (which is your case)
And both 1) and 2) are mostly about a series of hacks to make it works for those particular scenarios. So we are well aligned here I think :P
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853594663
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640502002
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
+ """
+ Returns true if executing system is Windows
+ """
+ return platform.system() == 'Windows'
Review comment:
We don't need a function for this either.
```suggestion
IS_WINDOWS = platform.system() == 'Windows'
"""True if operating system is Windows"""```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640625247
##########
File path: airflow/models/baseoperator.py
##########
@@ -955,7 +955,7 @@ def __deepcopy__(self, memo):
for k, v in self.__dict__.items():
if k not in shallow_copy:
- setattr(result, k, copy.deepcopy(v, memo)) # noqa
+ setattr(result, k, v if type(v).__name__ == 'module' else copy.deepcopy(v, memo)) # modules cannot be pickled
Review comment:
Good point. I am not sure if this comes from the different implementations of some dependencies between Windows and the other systems. This was basically one approach to get rid of the errors popping up without having to go through all the offending attributes. I will see if I can make this a bit less "hacky".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-943219956
@casra-developers Before making a rebase, it is worthwhile to merge all the changes.
```
git rebase -i HEAD^^^^^^^^^^^
```
And then you need to replace all the `pick` commands with `fixup` commands except the first one. You will have to use `git push --force` when pushing the change.
If you have a new branch that you want to put in this PR then you can just overwrite all commits in that PR. You don't have to create new PR.
```shell
git checkout old-branch
git reset --hard new-branch
git push --force
```
Here are tips from our [pull request guideline](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines):
> When merging PRs, Committer will use Squash and Merge which means then your PR will be merged as one commit, regardless of the number of commits in your PR. During the review cycle, you can keep a commit history for easier review, but if you need to, you can also squash all commits to reduce the maintenance burden during rebase.
Are these tips helpful to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Anurag-Shetty commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
Anurag-Shetty commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-953748229
i tried checking out **casra-developers:master** branch and tried running sample task, i got the below error for logs.
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: 'c:\\airflow\\logs\\SampleDag\\print_the_context\\2021-10-27T11:07:21.843119+00:00'
@casra-developers what am i missing here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-948556475
So I installed WSL2 on my machine and all the required extensions for running the hooks via pre-commit. The isort static-check should be fixed but now there is an issue during the Build Images step. I tried to follow all the docs (e.g. https://github.com/apache/airflow/blob/ed40c4b4adfb5736b07c08909ffb2d512d347105/STATIC_CODE_CHECKS.rst#pre-commit-hooks) but this is totally new territory for me (we use different devops solutions) and the time I have is sort of limited. It would be nice if someone could point me towards the proper solution to get this done. I think we are very close to getting this through, its just some details that are in the way now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-948749208
Looks like the merge wasn't performed cleanly and the code now contains Git conflict markers and is invalid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-988555675
@potiuk any news on the failing tests? I see that there are some conflicts again. I can resolve those but seeing how this would be about the 4th iteration it would be good to coordinate so we can merge as soon as the conflicts are resolved. Just let me know in a comment and I will try to schedule the merge accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-996284746
You got unlucky on temporary docker unavailability :(. But also there was linux-only static check fail, I added a fixup to ignore it and committed it to re-run. 🤞 again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-996909103
> Did another rebase +1
I tld you it gets the easier, the more you do it ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-995851966
Ah .. closed/reopened by accident :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-996168861
🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-875625692
@casra-developers I've got a WIP stash that fixes that issue, just trying to juggle it, two talks for Airflow Summit and other PR reviews 😁 .
I'll open a PR soon. _Very_ draft PR https://github.com/apache/airflow/pull/16860
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651721228
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
Ah cool. Did not know that (not a Windows user :D)
I thought it's a Java-specfic behaviour only (it was part of the specification from the beginning). So no problem here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r641322409
##########
File path: airflow/operators/python.py
##########
@@ -377,9 +377,20 @@ def execute_callable(self):
render_template_as_native_obj=self.dag.render_template_as_native_obj,
)
+ # find python executable folder
+ candidates = [os.path.join(tmp_dir, 'bin'), os.path.join(tmp_dir, 'scripts')]
+ python_folder = None
+ for candidate in candidates:
+ if os.path.isdir(candidate):
+ python_folder = candidate
+ break
+
+ if python_folder is None:
+ raise AirflowException(f'Unable to find python executable in "{tempdir}"')
+
execute_in_subprocess(
cmd=[
- f'{tmp_dir}/bin/python',
+ os.path.join(python_folder, 'python'),
Review comment:
Very nice, I didn't know about this function. Updated in the latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640482527
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -86,6 +87,10 @@ def __init__(self, local_task_job):
# pylint: disable=consider-using-with
self._error_file = NamedTemporaryFile(delete=True)
+
+ # HOTFIX: When reporting exceptions, this file was usually locked because it was still opened by this process
+ self._error_file.close()
Review comment:
This looks wrong. [The documentation says this would destroy the file immediately](https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile), rendering this attribute useless.
##########
File path: airflow/operators/python.py
##########
@@ -377,9 +377,20 @@ def execute_callable(self):
render_template_as_native_obj=self.dag.render_template_as_native_obj,
)
+ # find python executable folder
+ candidates = [os.path.join(tmp_dir, 'bin'), os.path.join(tmp_dir, 'scripts')]
+ python_folder = None
+ for candidate in candidates:
+ if os.path.isdir(candidate):
+ python_folder = candidate
+ break
+
+ if python_folder is None:
+ raise AirflowException(f'Unable to find python executable in "{tempdir}"')
+
execute_in_subprocess(
cmd=[
- f'{tmp_dir}/bin/python',
+ os.path.join(python_folder, 'python'),
Review comment:
I think this can be simplified with `shutil.which`. Instead of trying to find a parent directory to `os.path.join` later:
```python
candidate_dirs = [os.path.join(tmp_dir, "bin"), os.path.join(tmp_dir, "Scripts")]
python_executable = shutil.which("python", path=os.pathsep.join(candidate_dirs))
if not python_executable:
raise AirflowException(...)
execute_in_subprocess(cmd=[python_executable, ...])
```
Also, I feel the `AirflowException` should show `tmp_dir` instead. Showing `tempdir` can be misleading because the Python executable isn’t directly in the temp directory, but subdirectory `tmp_dir`.
##########
File path: airflow/utils/process_utils.py
##########
@@ -20,14 +20,20 @@
import errno
import logging
import os
-import pty
import select
import shlex
import signal
import subprocess
import sys
-import termios
-import tty
+from airflow.utils.platform_utils import is_windows
+
+if is_windows():
+ from airflow.windows_extensions import termios, tty, pty
+else:
+ import pty
+ import termios
+ import tty
Review comment:
I feel we should have a global shim (maybe a new `airflow.platform` that works similarly to `airflow.compat`) and use it in all places instead of doing `if is_windows()` everywhere.
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
+ """
+ Returns true if executing system is Windows
+ """
+ return platform.system() == 'Windows'
Review comment:
Would be a good idea to `functools.lru_cache` this.
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -133,16 +138,28 @@ def run_command(self, run_with=None):
self.log.info("Running on host: %s", get_hostname())
self.log.info('Running: %s', full_cmd)
- # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
- proc = subprocess.Popen(
- full_cmd,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True,
- close_fds=True,
- env=os.environ.copy(),
- preexec_fn=os.setsid,
- )
+
+ if is_windows():
+ # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
+ proc = subprocess.Popen(
+ full_cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ close_fds=True,
+ env=os.environ.copy()
+ )
+ else:
+ # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
+ proc = subprocess.Popen(
+ full_cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ close_fds=True,
+ env=os.environ.copy(),
+ preexec_fn=os.setsid, # does not exist on Windows
+ )
Review comment:
I think we can use `start_new_session=True` instead. This options is a shorthand to `preexec_fn=os.setsid` if available, and silently ignored on Windows (if I’m understanding the documentation
##########
File path: airflow/utils/log/file_task_handler.py
##########
@@ -82,14 +83,28 @@ def _render_filename(self, ti, try_number):
'ts': ti.execution_date.isoformat(),
'try_number': try_number,
}
- return self.filename_jinja_template.render(**jinja_context)
-
- return self.filename_template.format(
- dag_id=ti.dag_id,
- task_id=ti.task_id,
- execution_date=ti.execution_date.isoformat(),
- try_number=try_number,
- )
+ result_name = self.filename_jinja_template.render(**jinja_context)
+ else:
+ result_name = self.filename_template.format(
+ dag_id=ti.dag_id,
+ task_id=ti.task_id,
+ execution_date=ti.execution_date.isoformat(),
+ try_number=try_number,
+ )
+
+ # replace ":" with "_" for windows systems
+ if is_windows() and ':' in result_name:
+ print(''.join([
+ 'WARNING: Log file template contains ":" characters ',
+ 'which cannot be used on Windows systems.\n\n',
+ 'Please modify "airflow.cfg":\n',
+ '\t e.g. log_filename_template = {{{{ ti.dag_id }}}}/{{{{ ti.task_id }}}}/{{{{ ts }}}}/{{{{ try_number }}}}.log -> ',
+ 'log_filename_template = {{{{ ti.dag_id }}}}/{{{{ ti.task_id }}}}/{{{{ ts | replace(":", "_") }}}}/{{{{ try_number }}}}.log\n\n',
+ 'The offending ":" have been replaced with "_" characeters but note that this might cause other systems ',
+ 'which are configured differently to not find the log files.'
+ ]))
+ result_name = result_name.replace(':', '_')
+ return result_name
Review comment:
I wonder if we should just always replace `:` everywhere. It’s only there because of the datetime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640486068
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -133,16 +138,28 @@ def run_command(self, run_with=None):
self.log.info("Running on host: %s", get_hostname())
self.log.info('Running: %s', full_cmd)
- # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
- proc = subprocess.Popen(
- full_cmd,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True,
- close_fds=True,
- env=os.environ.copy(),
- preexec_fn=os.setsid,
- )
+
+ if is_windows():
+ # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
+ proc = subprocess.Popen(
+ full_cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ close_fds=True,
+ env=os.environ.copy()
+ )
+ else:
+ # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
+ proc = subprocess.Popen(
+ full_cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ close_fds=True,
+ env=os.environ.copy(),
+ preexec_fn=os.setsid, # does not exist on Windows
+ )
Review comment:
I think we can use `start_new_session=True` instead. This options is a shorthand to `preexec_fn=os.setsid` if available, and silently ignored on Windows (if I’m understanding the documentation correctly).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-875625692
@casra-developers I've got a WIP stash that fixes that issue, just trying to juggle it, two talks for Airflow Summit and other PR reviews 😁 .
~I'll open a PR soon.~ _Very_ draft PR https://github.com/apache/airflow/pull/16860
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849604736
> Some comments on the implementation style. I have a hunch the `windows_extensions` submodule could have legal implications but that’s another topic…
When we made this module internally we did not really consider this but you are right. I'm not sure about the nomenclature to be honest but maybe "win_extensions" or "nt_extensions" would be less problematic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986846900
Agreed. Especially for weird setups like ours where Linux and Windows systems have to work together, this can potentially be very useful.
I have noticed that there are four tests of type <b>www</b> which are failing. I'm assuming that this is something that has nothing todo with our changes, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-950620001
Thank you @ashb for updating the review status. There seems to be a problem with the "Quarantined tests", anything I can do on our side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-949424772
@uranusjr this did the trick! 👍
Thank you very much for helping me. I guess we should be all good then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-946987631
> I have noticed that there is one test failing. Is there something that needs to be changed for the merge to commence?
Yep. You need to fix static-checks. Actually if you install `pre-commit` it should fix those errors for you whe you run `git commit --amend` (just read what it prints and follow).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-968622155
Turns out the conflict is pretty big. Could you try to resolve it locally and push?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-963847912
I hope this 3 failing stages are not critical. It would be great to get this done until Friday because I will be away for three weeks after that. @potiuk let me know how I can assist the process if there is anything left to do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-962666226
@casra-developers - I think it's ALMOST good to go. You are however 163 commits behind main, so I'd feel safer if you rebase to latest main
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Anurag-Shetty commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
Anurag-Shetty commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-938532655
@casra-developers latest changes are there in casra-developers:master branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-891142262
@casra-developers - you need to also run `compile_assets` when you prepare the package - this will compila all the static assets that are currently missing for you.
Coincidentally missing instructions about that were just merged today to INSTALL docs :)
https://github.com/apache/airflow/blob/67cbb0f181f806edb16ca12fb7a2638b5f31eb58/INSTALL#L108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Anurag-Shetty commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
Anurag-Shetty commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-933152517
Have you looked at this link ?
https://stackoverflow.com/questions/63497235/airflow-scheduler-crashes-when-a-dag-is-run
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853618930
Thanks, that sounds good to me! We can perhaps put those in `airflow.utils.platform` (e.g. `set_timer`/`clear_timer`) and do the `IS_WINDOWS` check there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640676744
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -133,16 +138,28 @@ def run_command(self, run_with=None):
self.log.info("Running on host: %s", get_hostname())
self.log.info('Running: %s', full_cmd)
- # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
- proc = subprocess.Popen(
- full_cmd,
- stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT,
- universal_newlines=True,
- close_fds=True,
- env=os.environ.copy(),
- preexec_fn=os.setsid,
- )
+
+ if is_windows():
+ # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
+ proc = subprocess.Popen(
+ full_cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ close_fds=True,
+ env=os.environ.copy()
+ )
+ else:
+ # pylint: disable=subprocess-popen-preexec-fn,consider-using-with
+ proc = subprocess.Popen(
+ full_cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True,
+ close_fds=True,
+ env=os.environ.copy(),
+ preexec_fn=os.setsid, # does not exist on Windows
+ )
Review comment:
Seems to work, I will change it. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849466021
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r649426106
##########
File path: airflow/__main__.py
##########
@@ -34,6 +35,15 @@ def main():
os.environ['KRB5CCNAME'] = conf.get('kerberos', 'ccache')
os.environ['KRB5_KTNAME'] = conf.get('kerberos', 'keytab')
+ # if dags folder has to be set to configured value, make sure it is set properly (needed on Dask-Workers)
Review comment:
This is not the right fix for this.
There's a bug in a previous feature where the "dag folder" should be replaced with `DAG_FOLDER` which is then automaticall replaced, but this isn't working.
We should fix that rather than adding a new config and new way of making this work.
See https://github.com/apache/airflow/blob/9ba796ef40fe833aba58f5aa13a63587106d8ffd/airflow/utils/cli.py#L160-L167 for where the code is (the problem is on the command we send to the executor.)
https://github.com/apache/airflow/issues/8061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r650767364
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
If you [set up pre-commit](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#contribution-workflow), this (and other linter errors in this PR) can be more easily caught before you push the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-857935662
@casra-developers what's the next step for this? Do you need someone particular to take a look at your recent changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853599666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640640171
##########
File path: airflow/windows_extensions/pty.py
##########
@@ -0,0 +1,170 @@
+"""Pseudo terminal utilities."""
+
+# Bugs: No signal handling. Doesn't set slave termios and window size.
+# Only tested on Linux.
+# See: W. Richard Stevens. 1992. Advanced Programming in the
+# UNIX Environment. Chapter 19.
+# Author: Steen Lumholt -- with additions by Guido.
Review comment:
This file is copied from the installation directory of Python. Under Windows this is "...\Python37\Lib\pty.py". The license should be this one [https://docs.python.org/3/license.html](https://docs.python.org/3/license.html). We probably put it there to have all POSIX replacements in the same place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-849470512
Looks like a weekend adventure to take a look (and possibly finally use the dual boot dusted Windows partition on my Linux)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r651460996
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
Thanks for explaining. I was not familiar with mypy but this seems very useful also for future projects :)
I have committed the change request and there is now an Optional annotation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640632432
##########
File path: airflow/windows_extensions/Signals.py
##########
@@ -0,0 +1,35 @@
+import enum
+
+class Signals(enum.Enum):
Review comment:
True, this was basically just to satisfy some of the scripts in airflow.utils. We tried to approximate the behavior of the various signal method by using things like e.g. threads for timers. Certainly not ideal, I am very open to better options.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-995958879
@potiuk Resolved all conflicts and tested within our Linux-Windows setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r771000339
##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -19,6 +19,12 @@
import os
import subprocess
import threading
+
+from airflow.utils.platform import IS_WINDOWS
+
+if not IS_WINDOWS:
+ from pwd import getpwnam
Review comment:
```suggestion
# ignored to avoid flake complaining on Linux
from pwd import getpwnam # flake8: noqa
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-949459121
Yeah I think that's all I've got. @ashb will probably want to take some more look at this so let's wait for him to get back to this (I think he's still on vacation for ~1 week).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986812778
@uranusjr I have completed the manual merge of the main branch. Everything seems to work in the test environment, the only thing I have noticed that there seems to be a problem in the web interface in the tree view.
Keeps loading
<img width="1069" alt="image" src="https://user-images.githubusercontent.com/68997390/144861016-fd56dc4b-adae-452f-a399-f9628c40fbb1.png">
<img width="965" alt="image" src="https://user-images.githubusercontent.com/68997390/144861049-4aad56f7-773c-49ca-9fb3-6e2d526e8447.png">
I assume that this has nothing to do with our changes so I guess now would be a good time to complete the merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-953771845
@Anurag-Shetty I assume this is because Windows does not support ":" in filenames and folder paths. If you change your configuration to replace them with "_" or something else it should work.
In airflow.cfg, change
```
log_filename_template = {{{{ ti.dag_id }}}}/{{{{ ti.task_id }}}}/{{{{ ts }}}}/{{{{ try_number }}}}.log
```
to
```
log_filename_template = {{{{ ti.dag_id }}}}/{{{{ ti.task_id }}}}/{{{{ ts | replace(":", "_") }}}}/{{{{ try_number }}}}.log
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-988622762
Oh. yeah. a number of people have done quite some effort on stabilizing the tests and most of them are grreen now. So hopefully this is the last time because of unrelated test failures. Those were rather difficult to find and fix because they were side effects of interacting between seemingly unrelated tests.
Also we are in the process of fixing MyPy (which actually caused yoyr conflicts).
Sorry for continuous rebase, It does happen sometimes and it's pretty inevitable when you have 300 commits a month (10 a day average). There is no better way of doing it unfortunately. We have a beta change coming from Github - "Merge Queue" https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/using-a-merge-queue that is currently in limited public beta and when it is released, it should help us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640634576
##########
File path: airflow/windows_extensions/__init__.py
##########
@@ -0,0 +1 @@
+from airflow.windows_extensions.Signals import Signals
Review comment:
Uups, mistake
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-961840377
Any news on merging this PR? Anything needed from my side to proceed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-891525703
@potiuk thank you for the hint. I will try to do that on our test environment as soon as I have a little bit of time at my disposal (this week might be tight).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-853594663
How should we go about the licensing topic? Should I copy the license note from python.org somewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-943219956
@casra-developers Before making a rebase, it is worthwhile to merge all the changes.
```
git rebase -i HEAD^^^^^^^^^^^
```
And then you need to replace all the `pick` commands with `fixup` commands except the first one. You will have to use `git push --force` when pushing the change.
If you have a new branch that you want to put in this PR then you can just overwrite all commits in that PR. You don't have to create new PR.
```shell
git checkout old-branch
git reset --hard new-branch
git push --force
```
Are these tips helpful to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r641322848
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
+ """
+ Returns true if executing system is Windows
+ """
+ return platform.system() == 'Windows'
Review comment:
Obsolete now since we are using a constant in the latest commit but I will keep this in mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640640171
##########
File path: airflow/windows_extensions/pty.py
##########
@@ -0,0 +1,170 @@
+"""Pseudo terminal utilities."""
+
+# Bugs: No signal handling. Doesn't set slave termios and window size.
+# Only tested on Linux.
+# See: W. Richard Stevens. 1992. Advanced Programming in the
+# UNIX Environment. Chapter 19.
+# Author: Steen Lumholt -- with additions by Guido.
Review comment:
This file is copied from the installation directory of Python. Under Windows this is "...\Python37\Lib\pty.py". The license should be this one [https://docs.python.org/3/license.html](https://docs.python.org/3/license.html). We put it there to have our own tty module injected instead of the one it uses normally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r640506615
##########
File path: airflow/windows_extensions/__init__.py
##########
@@ -0,0 +1 @@
+from airflow.windows_extensions.Signals import Signals
Review comment:
Actually it’s `Signals` and `signal`, but that’s still (more?) confusing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r649694640
##########
File path: airflow/utils/process_utils.py
##########
@@ -79,7 +86,7 @@ def signal_procs(sig):
else:
raise
- if pgid == os.getpgid(0):
+ if IS_WINDOWS and pgid == os.getpid() or not IS_WINDOWS and pgid == os.getpgid(0):
Review comment:
Parentheses would be redundant as the "and" operation precedes "or" according to Boolean Algebra. (would be like a*b+c*d -> (a*b)+(c*d))
##########
File path: airflow/utils/process_utils.py
##########
@@ -155,11 +162,13 @@ def execute_interactive(cmd: List[str], **kwargs):
"""
log.info("Executing cmd: %s", " ".join(shlex.quote(c) for c in cmd))
- old_tty = termios.tcgetattr(sys.stdin)
- tty.setraw(sys.stdin.fileno())
+ if not IS_WINDOWS:
+ old_tty = termios.tcgetattr(sys.stdin)
+ tty.setraw(sys.stdin.fileno())
+
+ # open pseudo-terminal to interact with subprocess
+ master_fd, slave_fd = pty.openpty()
Review comment:
True, this is why the patch only works if this is being used as a Dask-Worker. I can add a check and raise a NotImplemented exception if the function is used on Windows. Would this be enough for the time being or can you tell me how to implement the Windows counterpart?
##########
File path: airflow/utils/timeout.py
##########
@@ -31,20 +34,34 @@ def __init__(self, seconds=1, error_message='Timeout'):
self.seconds = seconds
self.error_message = error_message + ', PID: ' + str(os.getpid())
- def handle_timeout(self, signum, frame): # pylint: disable=unused-argument
+ def handle_timeout(self, *args): # pylint: disable=unused-argument
"""Logs information and raises AirflowTaskTimeout."""
self.log.error("Process timed out, PID: %s", str(os.getpid()))
raise AirflowTaskTimeout(self.error_message)
def __enter__(self):
try:
- signal.signal(signal.SIGALRM, self.handle_timeout)
- signal.setitimer(signal.ITIMER_REAL, self.seconds)
+ if IS_WINDOWS:
+ if hasattr(self, TIMER_THREAD_ATTR) and getattr(self, TIMER_THREAD_ATTR) is not None:
+ getattr(self, TIMER_THREAD_ATTR).cancel()
+ timer = Timer(self.seconds, self.handle_timeout)
+ setattr(self, TIMER_THREAD_ATTR, timer)
+ timer.start()
Review comment:
I really like this approach, much cleaner than the current one. :)
##########
File path: setup.cfg
##########
@@ -156,6 +156,9 @@ install_requires =
typing-extensions>=3.7.4;python_version<"3.8"
unicodecsv>=0.14.1
werkzeug~=1.0, >=1.0.1
+ # needed for generating virtual python environments when running tasks
+ virtualenv>=20.4.3
+ psycopg2>=2.8.6
Review comment:
When testing we had an exception due to the module missing. Maybe there is a dependency that only needs it on Windows. Should I remove it and just manually install it if needed or is there an option to only install it on Windows?
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
Review comment:
Can you elaborate why?
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
+ self.seconds = seconds
+ self.error_message = error_message + ': Operation timed out.'
Review comment:
I omitted it since we are using threading tools here but it can be added again since there should still be the PID of the starting process.
##########
File path: airflow/utils/timeout.py
##########
@@ -22,12 +22,43 @@
from airflow.utils.platform import IS_WINDOWS
from airflow.exceptions import AirflowTaskTimeout
from airflow.utils.log.logging_mixin import LoggingMixin
+from typing import ContextManager, Type
-TIMER_THREAD_ATTR = '_timer_thread'
+_timeout = ContextManager[None]
-class timeout(LoggingMixin): # pylint: disable=invalid-name
- """To be used in a ``with`` block and timeout its content."""
+class _timeout_windows(_timeout, LoggingMixin):
+
+ def __init__(self, seconds=1, error_message='Timeout'):
+ super().__init__()
+ self._timer: Timer = None
+ self.seconds = seconds
+ self.error_message = error_message + ': Operation timed out.'
+
+ def handle_timeout(self, *args): # pylint: disable=unused-argument
+ """Logs information and raises AirflowTaskTimeout."""
+ self.log.error("Operation timed out.")
+ raise AirflowTaskTimeout(self.error_message)
+
+ def __enter__(self):
+ try:
+ if self._timer:
+ self._timer.cancel()
+ self._timer = Timer(self.seconds, self.handle_timeout)
+ self._timer.start()
+ except ValueError:
Review comment:
Good point. Removed it in __enter__ and __exit__
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r641323332
##########
File path: airflow/utils/platform_utils.py
##########
@@ -0,0 +1,16 @@
+import platform
+
+def is_windows() -> bool:
+ """
+ Returns true if executing system is Windows
+ """
+ return platform.system() == 'Windows'
Review comment:
True, has been changed in the latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-986859848
https://github.com/apache/airflow/runs/4430426331
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#discussion_r650821324
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -33,6 +33,10 @@
# subfolder in a code repository. This path must be absolute.
dags_folder = {AIRFLOW_HOME}/dags
+# Replaces the dags folder path for incomming commands to always use the
Review comment:
Just a note to remove that one when/if we fix the location to be relative to AIRFLOW_DAG
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-994984675
I feel your frustration in the question. So let me try to be helpful and explain a bit.
I'd advise you to continue rebasing and ping us when you see tests succed. There are conflicts now as you can see. And Ping everyone here when it succeds. In a way when you are a PR "author" it's part of the author's "job" to drag attention of others when you see your PR is ready to merge (green or you only see unrelated issues). This is the best you can do.
It becomes quite obvious if you try to put yourself in our shoes. For you this is only (or maybe one of a few) PRs that you care about. Yet we - as committers had 340 (!) PR merged overt the course of 1 month https://github.com/apache/airflow/pulse/monthly) . There are just a handful of commiters and this is ~20 PRs merged per working day. Some of them taking 20-30 comments on. As you can possibly imagine none of us is on a lookou to merge PRs at the moment they succeed. We have likely 100-200 completeed PR jobs a day. This is also a reason why sometimes main is broken as things slip through.
So it's simply much easier for you to pay an attention and ping use when you see things are "good" for the PR you "care" about.
I understand you would like to work in "fire and forget" mode, but simply this is difficult (but possibly merge queue feature which I wrote about earlier, will help with that).
Just as a general note, there is also the old saying In fact if something is more painful, do it more often. if you rebase more often, it pains less overall becaue a) you do it in smaller chunks, b) you learn how to do it fast. I've learned how to rebase my commits and resolve conflicts quickly during working on Airlfow.
So, apologies if it takes longer than you thought and that you have to do it several times, but this is how it works and best you can do is to "vet" your PR and be a little annoying (but not after some time - immediately when you see it is ready to merge).
I hope that is helpful and provides you the right context on why things are like that . Thanks for your understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #16110:
URL: https://github.com/apache/airflow/pull/16110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-997885025
:tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-995958879
@potiuk Resolved all conflict and tested within our Linux-Windows setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-996542898
Did another rebase 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] casra-developers commented on pull request #16110: Added windows extensions
Posted by GitBox <gi...@apache.org>.
casra-developers commented on pull request #16110:
URL: https://github.com/apache/airflow/pull/16110#issuecomment-955955306
I am getting many emails about build stages failing. Is there anything I can do from our side to assist the merge or are there steps that need to be taken in order to resolve those CI issues?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org