Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Add read lock to get_flag_keys method#35

Merged
schmit merged 5 commits intomainfrom
sps/get-keys-read-lock
May 2, 2024
Merged

Add read lock to get_flag_keys method#35
schmit merged 5 commits intomainfrom
sps/get-keys-read-lock

Conversation

@schmit
Copy link

@schmit schmit commented May 1, 2024


labels: mergeable

Fixes: #issue

Motivation and Context

Adding a read lock to the get_flag_keys method to ensure we do not hit a race condition. Also:

  • refactored the Read/Write lock class to use a context manager for more readable code
  • replace the LRUCache with a simple dictionary

How has this been tested?

Tests pass

@schmit schmit requested review from aarsilv and leoromanovsky May 1, 2024 02:47
Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Just commenting instead of approving as this has exceed my comfort level reading python 😅

def __init__(self, max_size: int):
self.__cache: LRUCache = LRUCache(maxsize=max_size)
def __init__(self):
self.__cache: Dict[str, T] = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this cache used? If it's for assignments I think we do want some sort of eviction

Copy link
Author

Choose a reason for hiding this comment

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

This cache is used to store the flag configuration. There is no cache for assignments for the Python SDK at the moment.

from contextlib import contextmanager

# Copied from: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch06s04.html
# Adapted from: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch06s04.html
Copy link
Member

Choose a reason for hiding this comment

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

Show me the LLM vectors

Copy link
Author

Choose a reason for hiding this comment

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

I tried that first, it went down a much more complicated path so I just added the contextmanager myself :)

@schmit schmit merged commit 87f423e into main May 2, 2024
@schmit schmit deleted the sps/get-keys-read-lock branch May 2, 2024 23:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants