Fix mismatch in full pipeline outputs

[russellb]

f37ecfc Fix mismatch in full pipeline outputs

commit f37ecfc
Author: Russell Bryant [email protected]
Date: Wed Jul 3 11:27:38 2024 -0400

Fix mismatch in full pipeline outputs

The full knowledge pipeline had `question` and `response` as output
columns, while the skills pipelines used `question` and `answer`.

`generate_data.py` currently expects `response` instead of `answer`.
Instead of having to deal with both, just standardize on `response`,
since that seems to be used more frequently. For example, various
prompt filenames have "response" in their names.

Signed-off-by: Russell Bryant <[email protected]>

[aakankshaduggal]

Thanks for the quick fix @russellb 🎉

[markmc]

The _get_response() method handles the differences in the expected output from the "simple" and "full" pipelines. It incorrectly assumed that the "full" pipeline provided a "response" column, but it is actually "answer".

I'll admit to being thoroughly confused, but ...

Are you sure it's not "response" for the full knowledge pipeline?

Where exactly in the code do you even check this for sure? I'm looking at output_cols in the LLMBlock configs ...

[russellb]

The _get_response() method handles the differences in the expected output from the "simple" and "full" pipelines. It incorrectly assumed that the "full" pipeline provided a "response" column, but it is actually "answer".

I'll admit to being thoroughly confused, but ...

Are you sure it's not "response" for the full knowledge pipeline?

Yeah ...

Where exactly in the code do you even check this for sure? I'm looking at output_cols in the LLMBlock configs ...

Yeah, it's in there ... but not at the end like you might expect.

sdg/src/instructlab/sdg/default_flows.py

Line 153 in bcb7974

"output_cols": ["question", "response"],

It's at the beginning. The rest of the pipeline is checking it to potentially be discarded, but the columns we care about in what makes it out the other end come from there.

[markmc]

The _get_response() method handles the differences in the expected output from the "simple" and "full" pipelines. It incorrectly assumed that the "full" pipeline provided a "response" column, but it is actually "answer".

I'll admit to being thoroughly confused, but ...
Are you sure it's not "response" for the full knowledge pipeline?

Yeah ...

Where exactly in the code do you even check this for sure? I'm looking at output_cols in the LLMBlock configs ...

Yeah, it's in there ... but not at the end like you might expect.

sdg/src/instructlab/sdg/default_flows.py

Line 153 in bcb7974

"output_cols": ["question", "response"],

It's at the beginning. The rest of the pipeline is checking it to potentially be discarded, but the columns we care about in what makes it out the other end come from there.

but ... that's response not answer?

[russellb]

The _get_response() method handles the differences in the expected output from the "simple" and "full" pipelines. It incorrectly assumed that the "full" pipeline provided a "response" column, but it is actually "answer".

I'll admit to being thoroughly confused, but ...
Are you sure it's not "response" for the full knowledge pipeline?

Yeah ...

Where exactly in the code do you even check this for sure? I'm looking at output_cols in the LLMBlock configs ...

Yeah, it's in there ... but not at the end like you might expect.

sdg/src/instructlab/sdg/default_flows.py

Line 153 in bcb7974

"output_cols": ["question", "response"],

It's at the beginning. The rest of the pipeline is checking it to potentially be discarded, but the columns we care about in what makes it out the other end come from there.

but ... that's response not answer?

Ughhhhh, you're right.

The original code was written against what I copied a link to (the knowledge pipeline).

The test I did with @aakankshaduggal was a skills addition, which uses "answer".

The real problem here is the two are different in several ways, both inputs and outputs, and I didn't notice until now. Thank you for being diligent here.

[markmc]

Cool, lgtm 👍

[russellb]

I changed this PR completely.

Now it addresses the real problem, which is that the knowledge vs skills pipelines had different outputs. The original PR here fixed skills, but would have broken knowledge.

I'm still trying to get CI in place that can catch this stuff ...

[oindrillac]

lgtm

[aakankshaduggal]

Tested and this works, thanks @russellb