Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SparkInK8s internal deprecations #38777

Merged

Conversation

Owen-CH-Leung
Copy link
Contributor

Part of the fixes for #38642

I executed pytest tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::test_spark_kubernetes_operator and pytest tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::test_spark_kubernetes_operator_hook. Both are running successfully without warnings. Executing with args -W default does raise warnings but they are not emitted into warnings.txt, so I believe they are warnings from 3rd party libraries.

Hence I believe these 2 are false alarms and can be removed safely, but If that's not the case, I'm happy to fix the warnings for these 2 tests in this PR

@Owen-CH-Leung Owen-CH-Leung marked this pull request as draft April 5, 2024 14:20
@Owen-CH-Leung
Copy link
Contributor Author

FYI when executing pytest for the first time, RemovedInAirflow3Warning is raised for these 2 tests, and they are actually coming from the use of SubDagOperator in example_subdag_operator (I believe this will cause multiple unit test to raise the same deprecation warning)

Given SubDagOperator will be deprecated, shall we just remove this example dag file ?

tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::test_spark_kubernetes_operator ========================= AIRFLOW ==========================
Home of the user: /root
Airflow home /root/airflow
Initializing the DB - first time after entering the container.
You can force re-initialization the database by adding --with-db-init switch to run-tests.
[2024-04-05T16:28:20.020+0000] {db.py:1682} INFO - Dropping tables that exist
[2024-04-05T16:28:20.348+0000] {migration.py:216} INFO - Context impl PostgresqlImpl.
[2024-04-05T16:28:20.348+0000] {migration.py:219} INFO - Will assume transactional DDL.
[2024-04-05T16:28:20.359+0000] {migration.py:216} INFO - Context impl PostgresqlImpl.
[2024-04-05T16:28:20.359+0000] {migration.py:219} INFO - Will assume transactional DDL.
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running stamp_revision  -> 677fdbb7fc54
ERROR [airflow.models.dagbag.DagBag] Failed to import: /opt/airflow/tests/dags/test_clear_subdag.py
Traceback (most recent call last):
  File "/opt/airflow/airflow/models/dagbag.py", line 346, in parse
    loader.exec_module(new_module)
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/opt/airflow/tests/dags/test_clear_subdag.py", line 66, in <module>
    daily_job = create_subdag_opt(main_dag=dag)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/airflow/tests/dags/test_clear_subdag.py", line 41, in create_subdag_opt
    return SubDagOperator(
           ^^^^^^^^^^^^^^^
  File "/opt/airflow/airflow/models/baseoperator.py", line 488, in apply_defaults
    result = func(self, **kwargs, default_args=default_args)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/airflow/airflow/utils/session.py", line 79, in wrapper
    return func(*args, session=session, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/airflow/airflow/operators/subdag.py", line 100, in __init__
    warnings.warn(
airflow.exceptions.RemovedInAirflow3Warning: This class is deprecated. Please use `airflow.utils.task_group.TaskGroup`.

@Owen-CH-Leung Owen-CH-Leung marked this pull request as ready for review April 6, 2024 00:50
@eladkal eladkal requested a review from Taragolis April 7, 2024 06:11
@Taragolis
Copy link
Contributor

I guess until Airflow 3 we still need to have SubDag operator into the airflow/example_dags, it use in multiple places include into the documentation part. What we could do it is use some generic functions/fixtures which are ignore particular warning during fill DagBag/migration

We need to take into the account that some of the test was created years ago, and TBH most of the tests do not require a Dag and especially DagBag for the tests, so it could be rewritten to not to use this parts.

And last but not least, many tests use generic parts which are now deprecated, e.g. create DagRun without providing run_id. That mean a lot of test could be fixed by the same pattern.

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for about this PR, I have no objection to merge it. There are some of from this list might be False Negative (it is originally raise an error during the creation of this list) or already fixed somewhere

@Taragolis
Copy link
Contributor

And if someone interested why check failed into the most test case into the tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator

The problem with mocking deprecated module instead of actual use in

) # , return_value='default')
@patch("airflow.providers.cncf.kubernetes.operators.kubernetes_pod.KubernetesPodOperator.cleanup")

================================================ short test summary info ================================================
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_create_application_from_yaml_json - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_new_template_from_yaml - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_template_spec - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_env - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_volume - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_pull_secret - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_affinity - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_toleration - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
FAILED tests/providers/cncf/kubernetes/operators/test_spark_kubernetes.py::TestSparkKubernetesOperator::test_get_logs_from_driver - airflow.exceptions.AirflowProviderDeprecationWarning: This module is deprecated. Please use `airflow.providers.cncf.kubernetes.operators.pod` instead.
============================================= 9 failed, 1 warning in 7.30s ==============================================

@Taragolis Taragolis added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 8, 2024
@Taragolis Taragolis merged commit 87fc581 into apache:main Apr 8, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants