Introduce a sentinel value _NO_VALUE to improve Global resolvers to support defaults 0 or None

[ankatiyar]

Description

Allow passing None values as default for globals

Development notes

There's two things reported by a user -

1. There was a bug which wouldn't allow a zero value to be set for globals - example globals.yml

   maxLookBackDays: 0  --> Doesnt work
   maxLookBackDays: 1  --> It works
   

2. None default value support

If we can get a null/None support that would also be nice as i wanted to pass null as default in some cases but it wasn't possible

We do allow parameters to be set as None, so I feel like this was reasonable. I've added a logger warning if a None default value is being used for a global variable used somewhere.

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[noklam]

Why do we care about the value here? It could be anything included None or 0. Shouldn't we check where the key is a valid key instead of the value?

[ankatiyar]

I've made some changes!

[noklam]

We should fix this before we release. cc @astrojuanlu

[astrojuanlu]

I don't fully understand the context of this one. I get this part

maxLookBackDays: 0  --> Doesnt work
maxLookBackDays: 1  --> It works

but not part (2) or your comment @noklam , could you elaborate?

[ankatiyar]

@astrojuanlu First part is a bug which definitely needs fixing. The second part was that if someone has set their default value to None, it shouldn't error out and just set the value of the parameter to None -

my_param: "${globals:whatever, None}"

[noklam]

@astrojuanlu My comment is more on the implementation side, the bug is a result of missing conditions on "value". My point is we shouldn't validate the value (It can be 0 or None, it's a bug now because we use not), we should check if the "key" exists and assign the value as is.

[merelcht]

LGTM! 👍 I agree this should go into the release.

[noklam]

I saw it has 2 approval already, but I think there are still bugs. I am working on a draft to fix this.

[noklam]

Cc @SajidAlamQB #2983

[noklam]

I think we should have a sentinel value NO_VALUE rather than None to differentiate a user define default None versus a no-op

1. ${globals: DUMMY_KEY, None}
   2.${globals: DUMMY_KEY}

Right now we cannot differentiate between the two. When the globals is missing, 2. should raise an error while 1 should not (

I can push the new change soon. See more details in this PEP https://peps.python.org/pep-0661/

[noklam]

Found more bug and I am adding more tests to catch all the error.

1. c80437f - for handling non-existing key
2. b36ffbd - to handle a special case when the globals sub-key is not a dictionary.

I wonder can we handle this better with omegaconf, similar to the way we parse --params because we are running into the same traps that we need to handle dict and traversing the nested key ourselves.

[noklam]

I made 2 changes in the implementation and add few more regression tests.

1. Use omegaconf so we don't need to our resolve logic, the current one is buggy.
2. Use _NO_VALUE value to replace the None default.

re-request @SajidAlamQB and @merelcht because I added fair amount of changes.

[merelcht]

Thanks for digging a bit deeper in this and finding all edge cases @noklam ⭐ It looks like it needs some more test coverage. I'm happy to approve after that.

[merelcht]

Nice and elegant solution 👍

[SajidAlamQB]

I am happy with this very nice work @ankatiyar and @noklam! 🌟

[merelcht]

I left a minor comment about the error message, but other than that this looks good! Great work @ankatiyar and @noklam ⭐

[merelcht]

Is this error also thrown if the key isn't found? In that case the message should probably be as before: "Globals key '{variable}' not found and no default value provided. "

[noklam]

Yes. Currently there are 3 paths it can take:

1. exists in globals.yml (take the global value as is)
2. Not define in globals.yml, but default provided
3. Not define in globals.yml and not default provided - raise InterpolationResolutionError

[ankatiyar]

Done