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

Fixed metric adaptation issue with the prompt factory #1187

Conversation

ssoima
Copy link
Contributor

@ssoima ssoima commented Aug 10, 2024

Issue:
Runtime metric language adaptation (e.g. adapting to italian and then to german in the same run) does not work.

The reason is that the @DataClass factories in the metrics use singleton objects (which only get initialized when the module is loaded) => when the prompt is instantiated a second time, the same object gets returned. Check out this loom for a debug preview: https://www.loom.com/share/2a500a4d83064489936886674e8641f9?sid=682f87fc-4052-4a33-89e4-5e4d1a6663dd

Solution:
Changed the prompts to factory methods that returns a new prompt at every call

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Aug 10, 2024
Comment on lines 163 to 164
long_form_answer_prompt: Prompt = field(
default_factory=lambda: LONG_FORM_ANSWER_PROMPT
default_factory=long_form_answer_prompt_factory
Copy link
Member

Choose a reason for hiding this comment

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

what if we make a factory inside Prompt object and call it init() or factory().

Copy link
Contributor Author

@ssoima ssoima Aug 16, 2024

Choose a reason for hiding this comment

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

I think init goes a bit against the scope of __init__ and is misleading. The cleanest way would be to create a PromptFactory class, but that would add more complexity, so I'd say having a factory method is a good middle-way.

@ssoima ssoima force-pushed the feature/fix-language-adaptation-prompt-factory-issues branch from 8dc528c to 79c7683 Compare August 16, 2024 10:42
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Aug 16, 2024
@ssoima
Copy link
Contributor Author

ssoima commented Aug 16, 2024

@jjmachan , the failing lint check is not related to my code changes

@ssoima ssoima requested a review from jjmachan August 17, 2024 08:31
@ssoima
Copy link
Contributor Author

ssoima commented Sep 24, 2024

@jjmachan are you still following this?

@jjmachan
Copy link
Member

jjmachan commented Nov 3, 2024

hey @ssoima with the new Prompt object this has been fixed

closing this for now but I'm really sorry we couldn't merge it 🙁 but at the same time thanks a million for taking the time to raise this, really grateful too and do checkout this form https://docs.google.com/forms/d/e/1FAIpQLSdM9FrrZrnpByG4XxuTbcAB-zn-Z7i_a7CsMkgBVOWQjRJckg/viewform - our way of saying thank you 🙂

@jjmachan jjmachan closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants