You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "topherinternational (via GitHub)" <gi...@apache.org> on 2024/09/06 11:35:42 UTC
[PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
topherinternational opened a new pull request, #42066:
URL: https://github.com/apache/airflow/pull/42066
#42054 changed Dataset extra default from `None` to `{}`, HookLineageCollector's dataset creation function didn't change to conform to the new default. Failure observed in #42059 and elsewhere in tests.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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 [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333933159
> def create_dataset(*, path: str, extra={}) -> Dataset:
Can’t do this because Python… https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
Keeping the default as None and changing it to `{}` should be the way to do. I think I’ll add post-processing code in Dataset so None is converted automatically to `{}` so the interface does not change. (Once the Dataset class is created, there shouldn’t be compatible issues.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333897091
@uranusjr idk where the dataset `factory` signature is documented, but it should be documented that the `extra` kwarg should be changed from None to {}.
e.g. here is the common/io/file one:
`def create_dataset(*, path: str, extra=None) -> Dataset:`
When #42054 is re-implemented it should be swapped to
`def create_dataset(*, path: str, extra={}) -> Dataset:`
and so forth for others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333863003
@potiuk here's something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333882074
> This should also include the previous changes since they were reverted.
I opened this PR before @potiuk reverted 😓
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333871893
This wasn't covered by a test? We should add one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to make dataset objects totally unhashable, redux [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #42066:
URL: https://github.com/apache/airflow/pull/42066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333945799
I’ve re-applied the previous changes here, and also added a fix so callers (including HookLineageCollector) can continue to pass in a None. It will be converted automatically to a dict.
Can you work on a test to cover the `dataset_factory` code path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to make dataset objects totally unhashable, redux [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2334674109
I added "full-tests-needed" label and closed/reopened to re-run all tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333881083
> This wasn't covered by a test? We should add one.
Well it's revealed by some provider tests that look like they weren't run in #42054's checks.
I think the challenge is that this is arg defaults all the way down, there's a long chain of `func(kwargs, extra=None)` calls that would all need tests to detect if a call that passed no extras produced a dataset object that matched `Dataset(args)` (w/o an `extra` arg).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2333877212
This should also include the previous changes since they were reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `Dataset` `extra` arg default in `HookLineageCollector` [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on PR #42066:
URL: https://github.com/apache/airflow/pull/42066#issuecomment-2334489099
> I’ve re-applied the previous changes here, and also added a fix so callers (including HookLineageCollector) can continue to pass in a None. It will be converted automatically to a dict.
>
> Can you work on a test to cover the `dataset_factory` code path?
I removed my original change, I think it's best if we keep the call stack as it is and use your fix that coerces the None to {} only in the Dataset constructor, less confusing and easier to test that way imo.
I added more tests to create_dataset that cover the dataset_factory API (uri, kwargs, missing factory, and factory exception are covered).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Try to make dataset objects totally unhashable, redux [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #42066: Try to make dataset objects totally unhashable, redux
URL: https://github.com/apache/airflow/pull/42066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org