Remove the unnecessary SDG class

[markmc]

Part of #61 and instructlab/dev-docs#113

A pipeline chains together a sequence of blocks, and an SDG chains together a sequence of pipelines. There is no need for this additional layer - we can construct a pipeline with the full sequence of blocks, and not chain pipelines together.

Check out how the API is used in generate_data.py to get a sense of the improvement

[russellb]

Assuming e2e passes, I am definitely supportive of this general direction. There's some discussion on #61 with @shivchander, so I'm not going to approve/merge just yet to give time for his input.

[mergify]

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

[markmc]

Assuming e2e passes, I am definitely supportive of this general direction. There's some discussion on #61 with @shivchander, so I'm not going to approve/merge just yet to give time for his input.

The discussion has stalled for now

I've reduced the scope of this PR to just the SDG thing, since the rest is now incorporated into #86

[russellb]

I kicked off the CI job that runs the full pipeline.

Otherwise, this looks like a nice simplification to me.

[github-actions]

E2E (NVIDIA A10G x4 - full pipeline) workflow launched on this PR: View run

[github-actions]

e2e workflow failed on this PR: View run, please investigate.

[hickeyma]

@shivchander you raised reasons for maintaining the SDG class in #61 (comment). What do you think of the changes in this PR?

[markmc]

See also instructlab/dev-docs#109

[hickeyma]

See also instructlab/dev-docs#109

@markmc do you mean instructlab/dev-docs#113 instead?

[markmc]

See also instructlab/dev-docs#109

@markmc do you mean instructlab/dev-docs#113 instead?

Doh, yes - thank you

[mergify]

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

[hickeyma]

@markmc Is this blocked waiting on the design proposal to progress?

[markmc]

@markmc Is this blocked waiting on the design proposal to progress?

Thanks for the reminder. I've rebased

I think the only blocker is that @shivchander has not yet acked my YAGNI argument

[mergify]

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

[shivchander]

On the research side, we are actively working on developing complex flows that chain pipelines in a DAG fashion. Our goal with the SDG class wass to serve as an abstract wrapper capable of managing these complex flows.

Could this be renamed to something better? Yes. But I and the research team are not in favor of removing this, but I will not be a blocker on this PR, If there's a consensus on moving forward. Just wanted to give a heads-up that at the pace research moves, we might need something which can handle this sooner.

[markmc]

On the research side, we are actively working on developing complex flows that chain pipelines in a DAG fashion. Our goal with the SDG class wass to serve as an abstract wrapper capable of managing these complex flows.

Could this be renamed to something better? Yes. But I and the research team are not in favor of removing this, but I will not be a blocker on this PR, If there's a consensus on moving forward. Just wanted to give a heads-up that at the pace research moves, we might need something which can handle this sooner.

It's very difficult to remove an API once a library is starting to be used in the wild - and renaming an API is effectively removing an API. Library API maintainers need to be very careful about breaking downstream users. Removing an API generally involves a long deprecation period, to make sure users have time to migrate away from it.

I do not believe something called "SDG" will live on as the name of any abstraction in this library long-term, and I do not want to make it difficult to remove for no reason

Adding whatever the new abstraction should be is super easy, and made no more difficult by removing SDG

[mergify]

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

[markmc]

It's very difficult to remove an API once a library is starting to be used in the wild - and renaming an API is effectively removing an API. Library API maintainers need to be very careful about breaking downstream users. Removing an API generally involves a long deprecation period, to make sure users have time to migrate away from it.

We will release 0.2.2 or 0.2.3 any day now, which will be used by instructlab 0.18.0, and from that point on I expect we'll be much more cautious about incompatible API changes. For that reason, I'm going to go ahead with this change despite objections, on the basis that it is trivial to restore this abstraction when we need it.

[mergify]

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