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

chore: swap core for yggdrasil #322

Merged
merged 25 commits into from
Nov 18, 2024
Merged

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Nov 11, 2024

Swaps the core of the SDK for the Python bindings to Yggdrasil.

Currently this "works on my machine" because the engine isn't yet published, but all tests do pass with a local link.

Details

  • Feature and variant evaluations are all handled through Yggdrasil now
  • Metrics counting and retrieval is handled through Yggdrasil now
  • The feature loader has been pared down to a much simpler set of functionally, all state diffing is handled by the Rust layer of the engine (currently no diffing but its coming)
  • Custom strategies are handled through the Python layer of Yggdrasil now. This means an interface change for custom strategies
  • All hydration paths (bootstrap, load from cache, API hit) now return strings; parsing is handled by the Rust layer of Yggdrasil
  • Access to the features field has been left intact on the UnleashClient class but will now always return empty. This is intended to be swapped out for feature_definitions. features will be removed in a further clean up PR prior to release

Discussion points

  • The Yggdrasil engine is a bit more fussy about types, specifically on Variants. Yggdrasil returns a dataclass, rather than the untyped dict of the SDK. I've chosen to map this to the legacy format to allow consumers an easier time moving, while giving us an option to move to something a little more concrete at a later point
  • Custom strategies is a big change. This is going to require a major version and the new methods in Yggdrasil will error at initialisation time rather than warn at runtime. I think this is okay but I'm open to discussion here
  • The engine isn't hinted to be private (and it's not name mangled because that's horrible) because none of the other SDK fields are. I don't believe people should be rooting around with the engine directly but Python also tends to treat its users like responsible adults so I think that's maybe okay
  • I've chosen not to handle some of the YddrasilExceptions that can bubble from the engine, specifically for cases where this would be undefined behavior in the Rust layer. These Should Never Happen™ but if they do, it's not really safe for any healthy program to continue what its doing

"""
variant = self.engine.get_variant(feature_name, context)
if variant:
return {k: v for k, v in asdict(variant).items() if v is not None}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question. Yggdrasil core returns a dataclass here (which is like an object without methods, similar to a Rust struct). I'm a fan of dataclasses because they're quite explicit about their intent. The SDK at the moment returns an untyped dict. So I don't want to change it for 3 reasons:

  • Yggdrasil abstractions should stay in Yggdrasil, bleeding internal classes outside of the domain boundary is poor encapsulation
  • This is one less thing for consumers to have to think about or worry about, that one is quite important to me in Python because its quite hard to know all the places you may be using a return type. This means no one has to hunt down all their variants and change the way they access data on them
  • Dataclasses do optionals, which become nulls. The client spec suggests that variants without payloads shouldn't include that field rather than including a null. The dict comprehension here is filtering out nulls

feature_name,
)
variant = DISABLED_VARIATION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be cleaner to move this part over to _resolve_variant and make it always return a variant instead of either a variant or None?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it alone because I was going to implement the fallback function, which needs to know if there's a variant or not... then I realised we never got around to implementing that. So I'm leaving this until we do actually implement it

UnleashClient/loader.py Outdated Show resolved Hide resolved
UnleashClient/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a few nitpicks it seems to me like you're doing all the right things 👍

Copy link
Collaborator

@ivanklee86 ivanklee86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some random thoughts:

  • Should update the development docs on what yggdrasil is/how to upgrade.
  • Custom strategy changes probably need updates.

@sighphyre
Copy link
Member Author

Some random thoughts:

* Should update the development docs on what yggdrasil is/how to upgrade.

* Custom strategy changes probably need updates.

Cheers, both are absolutely on my list of things to do. I just wanted to keep this PR as narrow as possible. Will definitely be a thing prior to an actual release

@sighphyre sighphyre force-pushed the chore/swap-core-for-yggdrasil branch from 30e9cf7 to 6e92e16 Compare November 13, 2024 13:34
@sighphyre sighphyre force-pushed the chore/swap-core-for-yggdrasil branch from 6824042 to 490e1a1 Compare November 14, 2024 16:54
@sighphyre sighphyre marked this pull request as ready for review November 15, 2024 09:43
@coveralls
Copy link

coveralls commented Nov 15, 2024

Coverage Status

coverage: 95.923% (-0.6%) from 96.545%
when pulling c657db3 on chore/swap-core-for-yggdrasil
into fd59b80 on main.

@sighphyre sighphyre merged commit fa88e22 into main Nov 18, 2024
43 checks passed
@sighphyre sighphyre deleted the chore/swap-core-for-yggdrasil branch November 18, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants