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

fixes config injection edge cases #1430

Merged
merged 16 commits into from
Jun 5, 2024

Conversation

rudolfix
Copy link
Collaborator

@rudolfix rudolfix commented Jun 1, 2024

Description

This PR fixes two bugs (or unexpected behaviors):

  1. Inconsistent behavior when None is passed to dlt.source, dlt.resource and with_config decorated function that contains arguments with defaults that are not None.

Code below

@dlt.resource
def open_parquet(path, flavour="spark"):
   print(flavour)
   ...

when called open_parquet("file.parquet", None) will not reset flavour to None due to how resolve_config is implemented. Explicit None is ignored and you get spark or configured value instead.

This PR correct this behavior. Now in the call above spark argument evaluates to None as you should expect from regular Python call.

this is behavior change previously you could use None to tell that certain argument should be injected from config. Now the same behavior can be requested via

  • open_parquet("file.parquet") - skipping the default arguments. this is how what we show in our docs
  • open_parquet("file.parquet", flavour=dlt.config.value) - explicitly request injection
    (both worked in old dlt and will work here as well)

@z3z1ma you could be interested in this. if you wrap such decorated function in other functions and use None in the outer function signature with the intention to inject value from configuration - this will stop working. We had this issue for example here:

def package_runner(
    venv: Venv,
    destination_configuration: DestinationClientDwhConfiguration,
    working_dir: str,
    package_location: str,
    package_repository_branch: str = dlt.config.value,
):
   # calls decorated function

(previously None was used as default for package_repository_branch)

  1. Incremental wrappers were shared across resource instances.

A good example of such undesired behavior is sql_table resource in sql_database. When called to get an instance for a particular table with the incremental argument present, it wraps it in the same instance. With many such resources used in parallel wrong incremental instances may be used.

Details of changes

This PR introduces several changes to how configuration is injected

  1. dlt.config.value and dlt.secrets.value are implemented as sentinel singletons, there's no need to unparse source code to detect them
  2. when calling source/resource/with_config decorated function, None can be passed as argument and it will be handled exactly like those are handled in Python (previously None was overwritten by injected value)
  3. injection can be requested by passing sentinel values as arguments
  4. wrapping resources in config and incremental wrappers is moved from decorator to resource class
  5. when resource is cloned, it is rewrapped. it observes config section changes and generates separate incremental wrappers (finally bug where wrappers were shared by resources coming ie from sql_table is fixed)
  6. there are many tests added to config injection

Copy link

netlify bot commented Jun 1, 2024

Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit a77a85c
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/665ef0ec6641180008797650
😎 Deploy Preview https://deploy-preview-1430--dlt-hub-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rudolfix rudolfix self-assigned this Jun 4, 2024
@rudolfix rudolfix added the bug Something isn't working label Jun 4, 2024
@rudolfix rudolfix requested a review from sh-rp June 4, 2024 11:06
@rudolfix rudolfix marked this pull request as ready for review June 4, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants