Another small comment here (Kaxil - also addressing your concern) :). Sorry
- for not chiming in before - I was away for a few days and catching up :)
We can slightly modify the "example full test" to be more "executor
independent" as well.
1) we can move the os.environ to inside of the `main()` to handle the case
where we want to execute the test as python script.
if __name__ == "__main__":
os.environ['AIRFLOW__CORE__EXECUTOR'] = 'DebugExecutor'
test_run()
Or even eventually - if we see that Pytest alone is good enough we can even
get rid of the `main` case.
2) we can add `conftest.py` to `tests/system/providers` to also set those
variables for all test as auto-fixture of pytest (per test).
For example (not sure if the None case is well handled here):
@pytest.fixture(autouse=True)
def set_debug_executor(request):
old_executor = os.environ.get('AIRFLOW__CORE__EXECUTOR').
os.environ['AIRFLOW__CORE__EXECUTOR'] = 'DebugExecutor'
yield
os.environ['AIRFLOW__CORE__EXECUTOR'] = old_executor
This way we can achieve the executor to be set in both "main'' and "pytest"
case. BTW. I think relying on `pytest` as execution engine is much better
than relying on `airlflow test` (both for manual but especially for CI
cases where we have plenty of ready-to-use pytest extensions and features -
timeouts, parallel running, capturing stdout etc. etc. and perfect
integration with CI).
Being able to run `pytest tests/system/providers/google/cloud_build.py` to
run the test without any setup at all, where you have authentication set
for the machine. is the goal we want to achieve really (and this is what
the refactoring makes possible). We should not really need to use airflow's
CLI for that IMHO.
Pytest is very cool for that. For example we should be able to do this:
`pytest -n 30 tests/system/providers/google/ --timeout=600
---junit-xml=/tmp/output.xml --reruns 2` - This will un all google system
tests by a queue of 30 parallel test executors, will rerun each test up to
two times if they fail each test with 300 seconds maximum time of execution
and produce the xml output of the failures that we will be able to
visualise, browse and navigate (including seeing output of the tests only
in the tests that failed).
On Thu, Feb 24, 2022 at 2:48 PM Jarek Potiuk <[email protected]> wrote:
> One comment to Kaxil. Fully agree.
>
> I think that was a left-over from the previous attempt to define watcher.
> What we wanted to make sure is that all tasks are included when watcher
> dependency is added. This was mostly when we wanted to use:
> [task1, task2, task3] >> watcher
>
> This was really because we foresaw that someone can add new tasks, we do
> not forget to add it ito the list.
> But with `dag.list(tasks) >> watcher()`, it's not really needed. Our check
> for watcher will be much simpler:
>
> If watcher task (name convention) is added to example_dags, make sure that
> the last significant line of the example dag is `dag.list(tasks) >>
> watcher`
>
> That's about it. We do not need anything more and certainly there will be
> a number of example_dags that will have no watcher at all.
>
> J.
>
>
> czw., 24 lut 2022, 08:23 użytkownik Mateusz Nojek <[email protected]>
> napisał:
>
>> Hello again :)
>>
>> The vote has passed with 3 binding +1 votes (and 4 non-binding +1 votes):
>>
>> Jarek Potiuk +1 (binding)
>> Tomasz Urbaszek +1 (binding)
>> Kaxil Naik +1 (binding)
>> Bartłomiej Hirsz +1 (non-binding)
>> Eugen Kosteev +1 (non-binding)
>> Mars Group +1 (non-binding)
>> Łukasz Wyszomirski +1 (non-binding)
>>
>> Thanks everyone!
>>
>> We will proceed with the implementation.
>>
>> Kind regards,
>> Mateusz Nojek
>>
>>
>> śr., 23 lut 2022 o 21:04 Kaxil Naik <[email protected]> napisał(a):
>>
>>> Sounds good. While I don't think we should enforce the "watcher" task
>>> for all the example DAGs, it isn't a blocker.
>>>
>>> +1 (binding)
>>>
>>> On Tue, 22 Feb 2022 at 07:43, Mateusz Nojek <[email protected]> wrote:
>>>
>>>> Hello Kaxil and thanks for the questions.
>>>> Let me answer them...
>>>>
>>>> 1. We don't, but since we want to have one file that will be an
>>>> example DAG and also a system test at the same time, we need to decide
>>>> where to store them. We have 2 options: store them as tests somewhere in
>>>> `tests` directory or store them as example DAGs (e.g. in providers
>>>> directory). Since they fulfill their 'example' role using dynamic
>>>> documentation that is generated from code (no matter where the files are
>>>> stored), this purpose we have already covered. The 'test' role, on the
>>>> other hand, is fulfilled by storing them in the right place, indicating
>>>> that these are actually tests that can be executed. Also, we decided to
>>>> move them because since we are planning to do the redesign, we can find
>>>> a
>>>> better place for them. `tests/system/providers` seems like a proper
>>>> place
>>>> for system tests.
>>>> 2. I am not for or against setting up the DebugExecutor inside the
>>>> environment variable, but I would like to see strong arguments against
>>>> using it the way it is described in the AIP, so that it can possibly
>>>> break
>>>> the execution in some specific cases. Examples would be very desired.
>>>> 3. You're right, if all tasks do not have any trigger rules, we
>>>> don't need the watcher task. But since probably most of them will have
>>>> teardown tasks (tasks with trigger_rule set to "all_done" to do the
>>>> cleanup
>>>> after test) we need to introduce this watcher task to handle those. We
>>>> actually don't need to use the watcher task in DAGs without teardown
>>>> tasks,
>>>> but it's actually better to use it always because it doesn't provide
>>>> anything other than to support the possibility of having a teardown
>>>> task,
>>>> so if anyone in the future decides to add some task to remove some
>>>> resources, watcher task will be already there to handle it. It doesn't
>>>> bring that much complexity to the code and computational consumption.
>>>> Also,
>>>> we are going to have a precommit hook that will check against absence of
>>>> this piece of code and assure that it's there in every test. Of course,
>>>> it's only my opinion and if we later decide to not use watcher task
>>>> where
>>>> it is not essentially needed, we can avoid adding it, but having it
>>>> everywhere creates a kind of 'framework' for our tests and saves us from
>>>> possible mistakes.
>>>>
>>>> One of the concepts of the new design is to minimize the amount of
>>>> environment variables to maintain. When I was fixing and refactoring system
>>>> tests, the part that I found the most difficult was maintaining the list of
>>>> environment variables. Even if every possible variable has the "good"
>>>> default, it was not running successfully on the first run. I needed to
>>>> check all the values of variables and see if they are sensible. They
>>>> weren't. The problem is that they age very fast and with every change to
>>>> the Airflow, they become brittle, outdated, useless. One of the main
>>>> pillars of the new design of system tests is to minimize the number of
>>>> environment variables needed to be maintained so that the DAGs just work
>>>> without that much hassle required to configure them.
>>>>
>>>> I hope it delivers answers your concerns.
>>>>
>>>> Kind regards,
>>>> Mateusz Nojek
>>>>
>>>>
>>>> pon., 21 lut 2022 o 18:31 Kaxil Naik <[email protected]> napisał(a):
>>>>
>>>>> Apologies for chiming in late. I read the AIP now, please find my
>>>>> questions/comments below:
>>>>>
>>>>> 1. Why do we need to move the DAGs from example_dags ?
>>>>> 2. Hardcoding DebugExecutor using environment variables like that
>>>>> doesn't feel right. Might cause issues when running with other
>>>>> executors.
>>>>> 3. The Watcher task - why do we need that? If the example DAGs
>>>>> don't set any Trigger rules, the default Trigger rule is "all_success"
>>>>> so
>>>>> the DagRun will fail if any task fails.
>>>>>
>>>>> The only thing I think is needed to make the example DAGs actually run
>>>>> in CI/locally is making sure the values comes from Environment Variables
>>>>> while the DAG itself have sensible defaults.
>>>>>
>>>>> For instance, the following example should work fine if the
>>>>> environment variables are set.
>>>>>
>>>>> AIRFLOW__CORE__EXECUTOR=DebugExecutor
>>>>> python
>>>>> airflow/providers/google/cloud/example_dags/example_gcs_timespan_file_transform.py
>>>>>
>>>>> What would be ideal (according to me) is to have airflow test run work
>>>>> without execution_date which will take care of running it via
>>>>> DebugExecutor. (docs
>>>>> <https://airflow.apache.org/docs/apache-airflow/stable/cli-and-env-variables-ref.html#test>
>>>>> )
>>>>>
>>>>> This opens up the possibility for us in future to provide more useful
>>>>> debug logs (like import time, number of DAGs, warn when a DAG doesn't
>>>>> follow best practices etc) via the "airflow test" command. While this is
>>>>> out of scope of the current AIP, I feel it might be a great value add.
>>>>>
>>>>> Just my 2-cents.
>>>>>
>>>>> Regards,
>>>>> Kaxil
>>>>>
>>>>>
>>>>>
>>>>> On Mon, 21 Feb 2022 at 07:20, Mateusz Nojek <[email protected]>
>>>>> wrote:
>>>>>
>>>>>> Hello everyone,
>>>>>>
>>>>>> Since there was no response from the community during the weekend on
>>>>>> this voting, I am extending it for the next 48 hours, so it will end on
>>>>>> Wednesday 23rd February 9 AM CET.
>>>>>>
>>>>>> Current state is:
>>>>>> 2/3 required binding votes,
>>>>>> additional 4 non-binding votes.
>>>>>>
>>>>>> Kind regards,
>>>>>> Mateusz Nojek
>>>>>>
>>>>>>
>>>>>> niedz., 20 lut 2022 o 12:01 Mateusz Nojek <[email protected]>
>>>>>> napisał(a):
>>>>>>
>>>>>>> Hi everyone,
>>>>>>>
>>>>>>> It's less than 24 hours till the end of the voting.
>>>>>>> If anyone of you still hesitates, this is the time to share your
>>>>>>> opinion with a vote on this AIP :)
>>>>>>> Thank you!
>>>>>>>
>>>>>>> Kind regards,
>>>>>>> Mateusz Nojek
>>>>>>>
>>>>>>>
>>>>>>> pt., 18 lut 2022 o 21:36 Tomasz Urbaszek <[email protected]>
>>>>>>> napisał(a):
>>>>>>>
>>>>>>>> +1 (binding)
>>>>>>>>
>>>>>>>> Tomek
>>>>>>>>
>>>>>>>
