Skip to content

Marshalling scenarios #240

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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Marshalling scenarios #240

wants to merge 33 commits into from

Conversation

ericvergnaud
Copy link
Contributor

@ericvergnaud ericvergnaud commented May 19, 2025

@gueniai
The purpose of the PR is to demonstrate that support for weak typing (a fundamental feature of Python!) is a required feature when storing configs, by providing support for real life scenarios. The concern was raise as part of #189

To that end, this PR:

  • patches Installation.py to dynamically support or not support raw typing and weak typing
  • provides a series of real-life scenarios, each of which is run in the 3 modes (allowing or not raw and weak typing)

When running the test_marshalling_scenarios.py locally, you'll notice that:

  • all 8 scenarios pass when weak typing is allowed
  • only 2 scenarios out of 8 pass when only raw typing is allowed
  • only 1 scenario out of 8 passes when strict typing is required

I don't think it's realistic to expect that developers get things right upfront, we should therefore support their progress towards strict typing. That's how these features ended up here in the first place (I couldn't figure out a way to specify a recursive type for JSON), so it's very likely to happen again and again.

More importantly, it is quite likely that we will encounter scenarios where the data to be stored comes from a source we don't control, and it just needs to be passed through, see test_dynamic_config_data.

Finally, and critically, I also know from experience that as a team we will encounter orphan data, i.e. data that somehow got there, but the how is lost. Support for weak types is key to rapid resolution.

I am yet to see a scenario where supporting this would cause problems in the future, rather the opposite.

Hopefully this is convincing enough to force merge #189 (the PR is already approved, 'force' is only required because some pre-existing code does not pass the 'no-cheat' check, all other checks are green)

@sundarshankar89 FYI

ericvergnaud and others added 28 commits March 13, 2025 15:56
…tabrickslabs/blueprint into support-marshalling-of-object-and-any

* 'support-marshalling-of-object-and-any' of github.com:databrickslabs/blueprint:
  Initial test coverage for configuration marshalling/unmarshalling (#226)
  Fixed Blueprint Install (#225)
* main:
  Fix argument interpolation in colorised logs (#233)
  Ensure the non-colorized logs also include timestamps with second-lev… (#235)
  Test coverage for the customised logger setup (#232)
  Type hints for existing installation tests (#231)

# Conflicts:
#	tests/unit/test_installation.py
…-scenarios

* support-marshalling-of-object-and-any:
  fix unmarshalling of raw dict
  fix unmarshalling of raw list
…-scenarios

* support-marshalling-of-object-and-any:
  fix marshalling of None
  fix marshalling of None

# Conflicts:
#	src/databricks/labs/blueprint/installation.py
Copy link

github-actions bot commented May 19, 2025

✅ 40/40 passed, 2 skipped, 1m55s total

Running from acceptance #298

…-scenarios

* support-marshalling-of-object-and-any:
  formatting
  fix incorrect test
  fix crasher with union type
  handle invalid conversion

# Conflicts:
#	src/databricks/labs/blueprint/installation.py
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.

3 participants