search

flowersteam / lamorel

Lamorel is a Python library designed for RL practitioners eager to use Large Language Models (LLMs).
MIT License
190 stars 18 forks source link

A question on a `break` statement in `HF_LLM::forward()` #33

Open sadra-barikbin opened 7 months ago

sadra-barikbin commented 7 months ago

Hi there! I couldn't understand why a break has been used (not a continue) during creating batch_inputs and batch_outputs in the case there's no candidate for a context: https://github.com/flowersteam/lamorel/blob/b012e2e8f164145af80084955fc79344049bb9e1/lamorel/src/lamorel/server/llms/hf_llm.py#L255-L256

This skips the next contexts and they don't come into batch_inputs and batch_outputs. Is this the desired behavior? If yes, could you please tell why is so? Furthermore, it seems that we would face KeyError during creating batch_input_representations, this way. https://github.com/flowersteam/lamorel/blob/b012e2e8f164145af80084955fc79344049bb9e1/lamorel/src/lamorel/server/llms/hf_llm.py#L282-L286

as we're iterating over contexts including those we skipped earlier but there's no entry for them in _ids_tables.

@ClementRomac

ClementRomac commented 7 months ago

Hi!

Thanks for spotting this. This looks like a legacy code that shouldn't be there anymore. I think, at least for decoder-only models, we should create a single output with the last token of input to still be able to get activations when pre_encode_inputs=True. I guess the easiest fix would be to replace the break statement by _candidates = [""].

I need to properly test this to verify it is actually necessary.

ClementRomac commented 7 months ago

Hi,

I opened this PR which should handle empty candidates.