Skip to content

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 3, 2021

Extracted from #19825, these are required to make Mypy pass checking the two files.

@uranusjr uranusjr requested a review from ashb December 3, 2021 11:23
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 3, 2021
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@uranusjr uranusjr force-pushed the typing-models-xcom-skipmixin branch from 2e7f84b to 8ad8f4b Compare December 3, 2021 12:08
@ashb
Copy link
Member

ashb commented Dec 3, 2021

Tests are failing with "Object of type bytes is not JSON serializable" on trying to send XCom over the API.

Comment on lines +508 to +496
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be causing docs to fail too, as it's somehow finding two places for all the methods on XCom.

(I'm guessing as to cause, doc build is failing though)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is fine, the cause is actually the overloads:

WARNING: duplicate object description of airflow.models.xcom.BaseXCom.clear, other instance in _api/airflow/models/xcom/index, use :noindex: for one of them

File path: apache-airflow/_api/airflow/models/xcom/index.rst (215)

 210 |    .. classmethod:: clear(cls, execution_date: pendulum.DateTime, dag_id: str, task_id: str, session: Optional[Session] = None)
 211 | 
 212 | 
 213 | 
 214 |    
>215 |    .. classmethod:: clear(cls, execution_date: Optional = None, dag_id: Optional = None, task_id: Optional = None, *, run_id: Optional = None, session: Session)
 216 | 
 217 |       Clears all XCom data from the database for the task instance
 218 | 
 219 |       ``run_id`` and ``execution_date`` are mutually exclusive.

I think generated by sphinx-autoapi. I’d expect it to support this years old feature; it’s very disappoiting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well actually it does support it since last year, but we’re pinning an extremely old version.

Copy link
Member

Choose a reason for hiding this comment

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

Every so often I've tried upgrading sphinx-autoapi, but last n times I tried there was a bug that meant it didn't build docs on a more recent version :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are we using autoapi in the first place? Do we have any non-Python APIs that can’t be documented with sphinx.ext.autodoc?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is question mostly to @mik-laj who knows more of the context. But I share the same sentiment and experience as @ashb - any time I tried I backtracked so far. And yea - we are using extremely old version.

@uranusjr
Copy link
Member Author

uranusjr commented Dec 6, 2021

Wow autoapi 1.0 is so fucking ancient even my workaround does not work.

@uranusjr uranusjr force-pushed the typing-models-xcom-skipmixin branch from 8ad8f4b to dbc2769 Compare December 6, 2021 10:39
@uranusjr
Copy link
Member Author

uranusjr commented Dec 6, 2021

I added a hack to allow manually telling AutoAPI to ignore any code construct if the docstring contains :sphinx-autoapi-skip:. This is probably the best thing we can do, short of fixing overloads entirely.

Note that I chose to document the desired interface that uses run_id, instead of the actual interface that accepts both run_id and execution_date, because, well, we don’t want people to use execution_date if possible 🙂

@uranusjr uranusjr force-pushed the typing-models-xcom-skipmixin branch from dbc2769 to 39c553b Compare December 6, 2021 10:44
@ashb ashb force-pushed the typing-models-xcom-skipmixin branch from a13121d to 8bc8127 Compare December 6, 2021 22:05
@ashb
Copy link
Member

ashb commented Dec 6, 2021

I have just rebased this PR after the fix on main.

Plus some minor fixups to make the entire file pass Mypy.
This patch makes Sphinx-AutoAPI to skip anything if its docstring
contains ':sphinx-autoapi-skip:' (no quotes, surrounded by whitespaces).
@uranusjr uranusjr force-pushed the typing-models-xcom-skipmixin branch from 8bc8127 to 9057f74 Compare December 7, 2021 07:15
@uranusjr
Copy link
Member Author

uranusjr commented Dec 7, 2021

Rebasing (again) to hopefully fix the last of test failures.

@ashb
Copy link
Member

ashb commented Dec 7, 2021

✅ 🎉

@uranusjr uranusjr merged commit 6dd0a0d into apache:main Dec 7, 2021
@uranusjr uranusjr deleted the typing-models-xcom-skipmixin branch December 7, 2021 09:55
@potiuk potiuk added the mypy Fixing MyPy problems after bumpin MyPy to 0.990 label Dec 13, 2021
@kaxil kaxil added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 13, 2022
@jedcunningham jedcunningham added this to the Airflow 2.2.4 milestone Jan 27, 2022
jedcunningham pushed a commit that referenced this pull request Jan 27, 2022
jedcunningham pushed a commit that referenced this pull request Jan 28, 2022
jedcunningham pushed a commit that referenced this pull request Feb 17, 2022
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..) full tests needed We need to run full set of tests for this PR to merge mypy Fixing MyPy problems after bumpin MyPy to 0.990

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants