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

Use default_factory instead of calling the method in dataclass #38948

Merged

Conversation

hussein-awala
Copy link
Member

The method get_airflow_extras is called each time we import the module, which we can avoid by defining the attribute as a filed and setting the method as a default factory.

@Taragolis
Copy link
Contributor

AFAIK it is exact the opposite, default values assigned exactly once, as well as module imports exactly once, if do not count manual reload through importlib or clear sys.modules

The problem with callable on default arguments that if it becomes mutable it will have all side effects of mutable.

In the current proposal this function will call each time when new object of this dataclass instantiated

uranusjr
uranusjr previously approved these changes Apr 12, 2024
@uranusjr uranusjr dismissed their stale review April 12, 2024 06:31

From what I can tell, it is actually better to just call the function once. If we want to switch to default_factory (which is not unreasonable; import-time logic is bad in general), we need to cache the function.

@hussein-awala hussein-awala force-pushed the breeze/improve_BuildProdParams branch from 9bddc7a to bd55ba1 Compare April 14, 2024 17:06
@hussein-awala
Copy link
Member Author

AFAIK it is exact the opposite, default values assigned exactly once, as well as module imports exactly once, if do not count manual reload through importlib or clear sys.modules

The problem with callable on default arguments that if it becomes mutable it will have all side effects of mutable.

In the current proposal this function will call each time when new object of this dataclass instantiated

I agree with this, but running the method at import time is often (if not always) not recommended, I added a lru_cache as TP suggested. WDYT?

@potiuk
Copy link
Member

potiuk commented Apr 14, 2024

With cache it's good.

@hussein-awala hussein-awala merged commit 58820a9 into apache:main Apr 15, 2024
69 checks passed
@potiuk potiuk added this to the Airflow 2.9.1 milestone Apr 16, 2024
@jedcunningham jedcunningham removed this from the Airflow 2.9.1 milestone Apr 26, 2024
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.

5 participants