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

FF-3390 fix(python): fix typing information for Configuration.__init__ #43

Merged
merged 12 commits into from
Oct 15, 2024

Conversation

leoromanovsky
Copy link
Member

@leoromanovsky leoromanovsky commented Oct 15, 2024

🎫 Internal eppo ticket: FF-3390

Observation

When using Python on version 4.0.1 (latest), unable to initialization bandit configuration from external payload.

👉 Only bandit keys that have a corresponding key in the model configuration are available in the get_bandit_keys function.

Changes

  • Augments the python configuration wrapper to match the parameters of the rust library.
  • Adds unit test to verify the bandit configuration loading.
  • Bumped sdk-test-data submodule to latest.

@leoromanovsky leoromanovsky marked this pull request as ready for review October 15, 2024 05:19
@rasendubi rasendubi changed the title bug(python): add bandit_configuration parameter to python configuration initialization (FF-3390) FF-3390 fix(python): fix typing information for Configuration.__init__ Oct 15, 2024
Comment on lines 9 to 24
# Note: Before publshing v5.0.0
#
# Users current have to initialize the configuration like this:
#
# Configuration(some_bytes, some_other_bytes)
#
# In v5.0.0, we will add a named parameter to the constructor to make this
# more clear.
#
# Users will be able to initialize the configuration like this:
#
# Configuration(flags_configuration=some_bytes, bandits_configuration=some_other_bytes)
#
# Changes:
#
# def __init__(self, *, flags_configuration: bytes, bandits_configuration: bytes | None = None) -> None: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

"flags": {}
}).encode('utf-8')

FLAGS_CONFIG_WITH_BANDITS = ufc_with_bandits = json.dumps({
Copy link
Collaborator

Choose a reason for hiding this comment

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

double-assignment here is confusing

Copy link
Collaborator

@rasendubi rasendubi left a comment

Choose a reason for hiding this comment

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

Thanks for handling this!

@rasendubi
Copy link
Collaborator

Just to clarify what's happening here, it's not that "configuration wrapper" didn't have the necessary parameter — it's just typing information that was out-of-sync (.pyi files are just type hints). There's no actual behavior change in this PR.

@rasendubi rasendubi merged commit cbe97de into main Oct 15, 2024
23 checks passed
@rasendubi rasendubi deleted the lr/python-bandits-config branch October 15, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants