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

Persist DAG and task doc values in TaskFlow API if explicitly set #29399

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

josh-fell
Copy link
Contributor

@josh-fell josh-fell commented Feb 7, 2023

If users set doc_md arg on @dag- or @task-decorated TaskFlow functions and those functions have a docstring, the doc_md value is not respected. Instead this PR will enforce only using the TaskFlow function docstring as the documentation if doc_md (or any doc* attrs for tasks) is not set.

P.S. There were a number of DAG decorator tests that had improper assertions. Those have been fixed in this PR as well.

if self.function.__doc__:
op_doc_attrs = [op.doc, op.doc_json, op.doc_md, op.doc_rst, op.doc_yaml]
# Set the task's doc_md to the function's docstring if it exists and no other doc* args are set.
if self.function.__doc__ and not any(op_doc_attrs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking against all of the doc attrs for tasks rather than just doc_md. Thinking users most likely wouldn't set more than one of these at a time.

@@ -2565,58 +2565,49 @@ def noop_pipeline():

dag = noop_pipeline()
assert isinstance(dag, DAG)
assert dag.dag_id, "noop_pipeline"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typical example of improper assertion that is being fixed in this PR.


task
@dag_decorator(
"test-dag", start_date=DEFAULT_DATE, template_searchpath=template_dir, doc_md=template_file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was broken even after correcting the assertion. Just needed a proper template_searchpath arg.

If users set `doc_md` arg on `@dag`- or `@task`-decorated TaskFlow functions and those functions have a docstring, the `doc_md` value is not respected. Instead this PR will enforce only using the TaskFlow function docstring as the documentation if `doc_md` (or any `doc*` attrs for tasks) is not set.
@potiuk potiuk merged commit e02bfc8 into apache:main Feb 20, 2023
@eladkal eladkal added this to the Airflow 2.5.2 milestone Feb 20, 2023
@josh-fell josh-fell deleted the deco-docstrings branch February 21, 2023 03:02
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Feb 27, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
…9399)

If users set `doc_md` arg on `@dag`- or `@task`-decorated TaskFlow functions and those functions have a docstring, the `doc_md` value is not respected. Instead this PR will enforce only using the TaskFlow function docstring as the documentation if `doc_md` (or any `doc*` attrs for tasks) is not set.

(cherry picked from commit e02bfc8)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
…9399)

If users set `doc_md` arg on `@dag`- or `@task`-decorated TaskFlow functions and those functions have a docstring, the `doc_md` value is not respected. Instead this PR will enforce only using the TaskFlow function docstring as the documentation if `doc_md` (or any `doc*` attrs for tasks) is not set.

(cherry picked from commit e02bfc8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants