Skip to content

Verify decorators parameter in __init__.pyi matches operator #51048

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bowrna
Copy link
Contributor

@Bowrna Bowrna commented May 25, 2025

related: #48448
closes: #48448

Add tests to all existed decorators to make sure the parameter list they accept matches the operator.

In this PR, I have added tests to verify the match using the libcst library. i have included tests for few decorators and once i get reviews if this way of verifying is fine, i will add the other decorator case too. cc: @eladkal


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@uranusjr
Copy link
Member

I like the idea, but this feels like it belongs to a pre-commit check rather than a unit test.

@Bowrna Bowrna force-pushed the verify_parameters branch from efe07f1 to fc7db1f Compare May 26, 2025 07:06
@Bowrna
Copy link
Contributor Author

Bowrna commented May 26, 2025

I like the idea, but this feels like it belongs to a pre-commit check rather than a unit test.

yes that makes sense. let me add this as part of precommit test.

@Bowrna Bowrna force-pushed the verify_parameters branch 2 times, most recently from 87a9768 to 444ba87 Compare May 26, 2025 10:44
@uranusjr uranusjr changed the title add tests to verify existed decorators parameter in __init__.pyi matches with the operator to avoid missing out Verify decorators parameter in __init__.pyi matches operator May 26, 2025
@Bowrna
Copy link
Contributor Author

Bowrna commented May 29, 2025

@uranusjr There are some complexities in moving to pre-commit. As unit tests, they run within the virtualenv and are context-aware, running without issue. But to run as part of a pre-commit hook, setting up the path has to be done. even when done,

module = __import__(module_path, fromlist=[class_name])
This part of the code in the test loads and runs the module in real-time, triggering a missing dependency issue at runtime. how do you think this could be better handled?

@Bowrna
Copy link
Contributor Author

Bowrna commented Jun 3, 2025

@uranusjr when you have time, could you help me handle the above issue?

@Bowrna Bowrna force-pushed the verify_parameters branch from 444ba87 to a315bea Compare June 6, 2025 15:37
@Bowrna Bowrna force-pushed the verify_parameters branch from a315bea to fbe2ca0 Compare June 6, 2025 15:55
@Bowrna
Copy link
Contributor Author

Bowrna commented Jun 13, 2025

@amoghrajesh @potiuk @uranusjr If any of you get time, could you help me in understanding if this can be better handled?

@amoghrajesh
Copy link
Contributor

@Bowrna I also think its not a great idea to add this in unit tests but in precommit checks to catch things earlier and repeatedly.

Did you try using the inspect library and matching signatures.

Python 3.9.22 (main, Apr 30 2025, 00:01:18)
[GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>
>>>
>>> import sys
>>> import inspect
>>> from airflow.decorators import task
>>> from airflow.providers.cncf.kubernetes.operators.pod import KubernetesPodOperator
>>> op_sig = inspect.signature(KubernetesPodOperator.__init__)
>>> dec_sig = inspect.signature(task.kubernetes)
>>> op_params = {k for k in op_sig.parameters}
>>> dec_params = {k for k in dec_sig.parameters}
>>>
>>>
>>> diff_op = op_params - dec_params
>>> diff_dec = dec_params - op_params
>>>
>>>
>>> diff_op
{'volume_mounts', 'env_from', 'base_container_status_polling_interval', 'kubernetes_conn_id', 'ports', 'on_finish_action', 'do_xcom_push', 'cmds', 'arguments', 'image', 'random_name_suffix', 'name', 'dnspolicy', 'config_file', 'cluster_context', 'full_pod_spec', 'dns_config', 'labels', 'self', 'termination_message_policy', 'volumes', 'skip_on_exit_code', 'is_delete_operator_pod', 'init_container_logs', 'active_deadline_seconds', 'container_resources', 'logging_interval', 'container_security_context', 'affinity', 'startup_timeout_seconds', 'termination_grace_period', 'hostnetwork', 'callbacks', 'startup_check_interval_seconds', 'init_containers', 'image_pull_secrets', 'host_aliases', 'deferrable', 'base_container_name', 'log_pod_spec_on_failure', 'poll_interval', 'get_logs', 'pod_template_dict', 'security_context', 'in_cluster', 'configmaps', 'subdomain', 'pod_runtime_info_envs', 'service_account_name', 'hostname', 'secrets', 'container_logs', 'trigger_kwargs', 'schedule_timeout_seconds', 'log_events_on_failure', 'reattach_on_restart', 'image_pull_policy', 'tolerations', 'progress_callback', 'namespace', 'automount_service_account_token', 'node_selector', 'priority_class_name', 'env_vars', 'pod_template_file', 'schedulername', 'annotations'}
>>> diff_dec
{'multiple_outputs', 'python_callable'}

Tried with the console and it works pretty consistently.

@Bowrna Bowrna force-pushed the verify_parameters branch 4 times, most recently from 5945451 to 19f3114 Compare June 17, 2025 08:15
@Bowrna Bowrna force-pushed the verify_parameters branch 5 times, most recently from c3ec851 to a2a98fd Compare June 19, 2025 05:11
@Bowrna Bowrna requested a review from jedcunningham as a code owner June 19, 2025 05:11
@Bowrna Bowrna force-pushed the verify_parameters branch from 6d67af2 to 28243ff Compare June 19, 2025 10:09
@Bowrna Bowrna force-pushed the verify_parameters branch 3 times, most recently from 278eac1 to bc25384 Compare June 21, 2025 10:03
@Bowrna
Copy link
Contributor Author

Bowrna commented Jun 21, 2025

@uranusjr could you review this PR and share your feedback?

@Bowrna Bowrna force-pushed the verify_parameters branch 4 times, most recently from 20aaca2 to d7ffbc6 Compare June 24, 2025 07:15
@Bowrna
Copy link
Contributor Author

Bowrna commented Jun 25, 2025

@uranusjr the fix you suggested is added into this. thank you. when you get, please verify this PR and share your feedback.

@Bowrna
Copy link
Contributor Author

Bowrna commented Jun 30, 2025

@uranusjr do i have any update regarding this PR?

@Bowrna Bowrna force-pushed the verify_parameters branch 4 times, most recently from 957ae00 to 5ea92d3 Compare July 1, 2025 08:51
"short_circuit": "airflow.providers.standard.operators.python.ShortCircuitOperator",
"python": "airflow.providers.standard.operators.python.PythonOperator",
"external_python": "airflow.providers.standard.operators.python.ExternalPythonOperator",
# Add more here...
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think it would be great to have some way to detect when we need to add something here - likely importing everything and looking for anythiing deriving from decorator? We do similar things "verify providers" pre-comit check when we walk all classes and find ones that derive from BaseHook for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#51048 (comment)

I wonder how we can prevent people from forgetting. Maybe a separate pre-commit hook that checks if a commit adds an entry to the provider config (task-decorators in provider.yaml) but does not also change this? Or maybe we should just pull the list of classes from provider.yaml directly instead? This can probably be explored in a second PR after this is merged.

@uranusjr suggested this one @potiuk . could i add that as seperate PR after this one gets merged?

@Bowrna Bowrna force-pushed the verify_parameters branch from 5ea92d3 to d182838 Compare July 1, 2025 08:59
@Bowrna Bowrna force-pushed the verify_parameters branch from d182838 to bba86a1 Compare July 1, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test to verify parameters match between operator and decorator
4 participants