updates to grounded flow

[oindrillac]

Stress testing was done on the SDG pipeline

• added grounded test script that can be used to test
• updates to templates to get full pipeline working

[npalaska]

This change is somehow breaking the knowledge generation. The filter_relevancy block is returning a dataset with 0 rows (probably because it filters out all the responses generated by the previous block). Is this expected? Same goes for filter_verify_question block below.

[npalaska]

Ah this could be because the file src/instructlab/sdg/configs/knowledge/simple_generate_qa.yaml still contains

  {question_1}
  {response_1}

  {question_2}
  {response_2}

  {question_3}
  {response_3}
  

instead of

  {icl_query_1}
  {icl_response_1}
 
  {icl_query_2}
  {icl_response_2}

  {icl_query_3}
  {icl_response_3}

[russellb]

#57 addresses this

[markmc]

The commit log could be much improved. The logical changes I see, with the explanations I'd expect:

1. Convert filter values to a float - does this fix a bug? details of how the bug manifested would help others learn
2. Combine question and context in the grounded skills flow at the end - why? what's the before and after behavior
3. template updates to get full pipeline working - these look like important, but subtle changes, what details can you provide to help a person in future understand what to look out for if they are making further changes?

[oindrillac]

here are some explanations for the changes:

1. Convert filter values to a float - does this fix a bug? details of how the bug manifested would help others learn

This is addressing an issue that we observed. In some of the cases where the model is being asked to provide a total score, it provides floating point sums like 1.5/2.5 and to accommodate those cases where we want to filter those values out it made sense to keep it float.

2. Combine question and context in the grounded skills flow at the end - why? what's the before and after behavior

That is a step towards converting it to the final format (messages) required for training.

3. template updates to get full pipeline working - these look like important, but subtle changes, what details can you provide to help a person in future understand what to look out for if they are making further changes?

The template changes stem from model behavior on the templates and stress testing across various leaf nodes. The changes were mainly made to make sure the responses from the model align best with the expected behavior

[markmc]

here are some explanations for the changes:

It would be great to get those explanations into the commit messages. See also "Information in commit messages" in https://www.berrange.com/tags/commit-message/

The commit message must contain all the information required to fully understand & review the patch for correctness. Less is not more. More is more.

[shivchander]

here are some explanations for the changes:

It would be great to get those explanations into the commit messages. See also "Information in commit messages" in https://www.berrange.com/tags/commit-message/

The commit message must contain all the information required to fully understand & review the patch for correctness. Less is not more. More is more.

Thanks for the recommendation! We could start doing this here on, this PR needs to go in by today. We are on a very tight deadline

[shivchander]

a few minor changes, otherwise lgtm

[oindrillac]

a few minor changes, otherwise lgtm

done, thank you!

[aakankshaduggal]

LGTM, thanks @oindrillac

[shivchander]

LGTM, thanks

[npalaska]

LGTM

[russellb]

the last two commits just say "updated to string" but neither are updating something to be a string.

either way it looks like something that should be squashed in to your main commit

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. @oindrillac please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[oindrillac]

@russellb squashed

[russellb]

@russellb squashed

Thanks. Can you also add your explanations of the changes that you gave here to the commit messages as suggested?

[oindrillac]

@russellb @markmc updated the commit messages as per your suggestion, ptal thanks

[russellb]

I see that e2e passed, though the commit messages still seem the same? I'm happy for it to merge once that content is there. Let me know if any help is needed with that. You don't need to wait for e2e to pass again if you only change the commit message. Just merge away.

[russellb]

lgtm!

[oindrillac]

Oh for some reason it didn't get synced. Thanks for adding descriptive notes @aakankshaduggal