Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: move class-level attributes to instance-level in Conversation class #418

Merged

Conversation

acse-efk23
Copy link
Contributor

Fixes #417

Why:
Class-level attributes persisted state between sessions. Making them instance-level ensures each session starts fresh.

What:
Moved _thread, _should_stop, _conversation_id, and _last_interrupt_id from class-level to instance-level within the init method.

Note: This pull request is a proof of concept, demonstrating how moving class-level attributes to instance-level resolves the session interference issue. I understand this can’t be merged directly due to the code generation process.

@louisjoecodes
Copy link
Collaborator

Thank you @acse-efk23! @tudor11l, @lacop11 - do you think this is something we want to merge? Makes sense to isolate these at instance-level rather than class-level

@tudor11l
Copy link
Collaborator

@louisjoecodes we fixed the only one that was causing a problem which was the mutable (by not initialising it with a default value)
threading.Event = threading.Event()

I'm not a huge fan of class-level variables especially when they do not have the same value across instances. But it depends on what others think as well.

@lacop11
Copy link
Member

lacop11 commented Dec 13, 2024

Thanks @acse-efk23 !

We have fixed the mutable _should_stop issue (I didn't notice your PR earlier unfortunately, would save us some time debugging!) but I agree we need a more consistent approach.

Note that these are not class variables but instance variables, IIUC. See https://docs.python.org/3/library/typing.html#typing.ClassVar.

The problem was the mutable default value. I think we should be fine just dropping all default values and only keeping type hints on the class level, then putting all defaults into __init__. We can do it for the = None and = 0 even though they are not mutable and don't cause issues, just to be consistent and avoid accidentally adding more mutable defaults in future.

@acse-efk23 BTW:

I understand this can’t be merged directly due to the code generation process.
This particular file is not generated and is excluded in https://github.com/elevenlabs/elevenlabs-python/blob/main/.fernignore#L4 so it won't be overwritten.

If you would like to adjust the PR to keep the type hints as they were and just move the value assignment we would be happy to merge it. :)

@acse-efk23
Copy link
Contributor Author

Thank you for the review, @lacop11 . I have updated the pull request accordingly.

@lacop11
Copy link
Member

lacop11 commented Dec 17, 2024

Looks good but the branch is out of date and conflicts, can you please rebase + resolve?

# Conflicts:
#	src/elevenlabs/conversational_ai/conversation.py
@acse-efk23 acse-efk23 force-pushed the fix/multiple-conversation-interference branch from cfb08c2 to 1733cd2 Compare December 17, 2024 13:38
@acse-efk23
Copy link
Contributor Author

Thank you, @lacop11 . I have rebased and solved the conflicts.

Copy link
Member

@lacop11 lacop11 left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@lacop11 lacop11 merged commit 95bd48e into elevenlabs:main Dec 17, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple conversations interfere due to class-level attributes
4 participants