[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1450811879 @BasPH Also one more case why having example_dags as "executable" and separate python file here: https://github.com/apache/airflow/pull/28325 Our example dags are actually imported at tested during the CI. Originally the taskflow example dag has failed when submitted and required some fixes in the test code. So it was a test issue not "airflow core" issue, however it could also be "core" issue, regression that could have been missed in unit tests and only surface at the moment real DAG is parsed/serialized. And IMHO it underlines how important it is to have the example dags as separate python files that can be both - run manually very easily and testsd automatically with every PR. In Polish, we have the idiom "wylać dziecko z kąpielą" - which freely translated means "to throw away the baby together with the bathing water". Trying to move example_dags to be just part of the docs IMHO is a very good example of that - by trying to improve the docs - the attempt here is to remove example python files that are giving us extra test harness that adds yet another level of making sure our code does what it should do. I'd say we should find a way to improve the docs while keeping the baby. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1438506435 > We should separate purposes and store code for docs separate from tests, i.e. in the /docs folder. Since that would be a bigger change, happy to close this PR for now and use it to demonstrate how that change might work. I think we agree on that. I am absolutely for separating those two use cases for users. Users when installing Airflow by default (for production) should not see those. But I think those `example_dags` (they can be renamed to `doc dags` or something should also be possible to bring back to be visible in the UI by simple "opt-in" flag, without manual copying those files to the dags folder of theirs. This is great value for people who are exploring airlfow and want to get the grasp of it. Equivakent to "playground" in a number of services that show how an application works. I think this is really valuable that the user workflow for such a "playful" user is: * `pip install airflow` * `airflow standalone --load-example-dags` And be able to run them. Same for `breeze` and local venv environment, where the contributor would be able to run those examples without copying them to their DAG folder. Then the benefit is that you can easily make sure that the dags are still working while you are making changes to some core classes - and that you won't forget to copy-back the changed/fixed example dags from your DAG folder when you are done (because you will be editing them in-place in your sources and commit together with the rest). So I have no problems with moving the dags to "docs" section, as long as the workflows above continue to work. It basically means that the example dags (or doc dags) will have to be embedded in airflow code when package is prepared (or taken from docs or whatever when run from developer's venv/breeze). -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1412956934 Oh yes - I absolutely agree example dags should not be loaded by default.This is even one of the reasons why in Breeze it is disabled by default. And if anything I would argue for changing the flag to False. Since this is not a production setting, I think we could flip it to False without breaking any "backwards compatibility promise". Maybe that is a better approach - keep the examples where they are but disable them. Or maybe even we could fine-grain it and enable/disable different types of the examples. That sounds like a better idea that introducing a new place where example dags are stored without the "easiness" of running them. I think you might not understood why I think it's good to have the code in example_dags and copied from there - it's not because it is a python file, but it is because I can easily run airflow wiith a flag and run them without even copy-pasting the code. This is for example how I as a maintainer test a new release of airflow. I run `breeze start-airflow --use-airflow-version 2.5.1rc1 --load-example-dags --load-default-connections` and then I run many of those examples - and by seing them succeed, I can be reasonably sure that our example docs are also "fine". But yes - keeping the flag disabled by default is a very good idea (like it is in breeze). -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1411034438 Exactly same effect, example is runnable with `--load-example-dags` flag in start-airflow. No code duplication. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1411031392 > I suggest showing reproducible examples. The current example for the PythonSensor is not easily reproducible because it doesn't show imports. Yes I understand that. What I am suggesting to also surround import with "exaampleinclude" start. There is no reason exampleicnlude start/end comments shoud be limited to in-dag. the new `example_python_sensors.dag` could look like this (and gradually we could extract other sensors in similar way to separate files): ``` # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file # distributed with this work for additional information # regarding copyright ownership. The ASF licenses this file # to you under the Apache License, Version 2.0 (the # "License"); you may not use this file except in compliance # with the License. You may obtain a copy of the License at # # http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, # software distributed under the License is distributed on an # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. # [START example_python_sensors] import datetime from airflow.decorators import dag, task from airflow.sensors.python import PythonSensor @dag(start_date=datetime.datetime(2023, 1, 1), schedule=None) def example(): # TaskFlow sensor @task.sensor def wait_for_success_taskflow(): return datetime.datetime.now().minute % 2 == 0 wait_for_success_taskflow() # Equivalent functionality using PythonSensor class def wait_for_success_pythonsensor(): return datetime.datetime.now().minute % 2 == 0 PythonSensor(task_id="wait_for_success_pythonsensor", python_callable=wait_for_success_pythonsensor) example() # [END example_python_sensors] ``` (whole file). The weekday -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1410355082 Very nice. Just a question - should not we make it back into the example_sensors.py and include from there? I think the big benefit of include_examples is that the documentaion can actually be tested very easily and in case we make any refactoring in the future it is far less likely to be "invalid" - and for example the discussion https://github.com/apache/airflow/pull/29143#discussion_r1086143815 would not be likely needed? I think the code could be simply copied into the example_sensors.py and end up with nearly the same result (barring the example_include header) Why not doing this way ? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1410354345 Very nice. Just a question - should not we make it back into the example_sensors.py and include from there? I think the big benefit of include_examples is that the documentaion can actually be tested very easily and in case we make any refactoring in the future it is far less likely to be "invalid" - and for example the discussion https://github.com/apache/airflow/pull/29143#discussion_r1086143815 would not be likely needed? I think the code could be simply copied into the example_sensors.py and end up with nearly the same result (barring the example_include header) Why not doing this way ? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #29143: Demonstrate usage of the PythonSensor
potiuk commented on PR #29143: URL: https://github.com/apache/airflow/pull/29143#issuecomment-1410353637 Very nice. Just a question - should not we make it back into the example_sensors.py and include from there? I think the big benefit of include_examples is that the doycumentaion can actually be tested very easily and in case we make any refactoring in the future it is far less likely to be "invalid" - and for example the discussion https://github.com/apache/airflow/pull/29143#discussion_r1086143815 would not be likely needed? I think the code could be simply copied into the example_sensors.py and end up with nearly the same result (barring the example_include header) Why not doing this way ? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
