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.

Sign up for GitHub

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

Jump to bottom

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

Open
sadra-barikbin opened this issue Feb 18, 2024 · 2 comments · May be fixed by #36
Open

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

sadra-barikbin opened this issue Feb 18, 2024 · 2 comments · May be fixed by #36
Assignees
@ClementRomac
Labels
enhancement New feature or request

Comments

@sadra-barikbin
Copy link
Contributor

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:

lamorel/lamorel/src/lamorel/server/llms/hf_llm.py

Lines 255 to 256 in b012e2e

if len(_candidates) == 0:
break

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.

lamorel/lamorel/src/lamorel/server/llms/hf_llm.py

Lines 282 to 286 in b012e2e

for step in range(len(contexts) // _minibatch_size + 1 ):
_input = {}
for _i in range(step*_minibatch_size, min((step+1)*_minibatch_size, len(contexts))):
if len(_ids_tables[_i]) == 0: break
_current_context = batch_inputs[_ids_tables[_i][0]]

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

@ClementRomac
@ClementRomac
Copy link
Collaborator

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 ClementRomac self-assigned this Feb 21, 2024
@ClementRomac ClementRomac added the enhancement New feature or request label Feb 21, 2024
@ClementRomac ClementRomac linked a pull request Feb 21, 2024 that will close this issue
Better handling of empty candidates in forward #36
Open
@ClementRomac
Copy link
Collaborator

Hi,

I opened this PR which should handle empty candidates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ClementRomac ClementRomac

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Better handling of empty candidates in forward flowersteam/lamorel
2 participants
@ClementRomac @sadra-barikbin