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.

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

pipeline: Fail explicitly on an empty dataset #127

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

russellb
Copy link
Member

@russellb russellb commented Jul 12, 2024

If at any point generate() on a block returns an empty dataset, this
is a failure condition. Go ahead and raise an exception right away in
that case.

This change was originally a subset of this commit: aakankshaduggal@256335e

Co-authored-by: shiv [email protected]
Co-authored-by: Aakanksha Duggal [email protected]
Co-authored-by: Kai Xu [email protected]
Signed-off-by: Russell Bryant [email protected]

@russellb russellb force-pushed the empty-dataset-exception branch from 8c50c7a to b812fa4 Compare July 12, 2024 18:08
@russellb
Copy link
Member Author

I pushed an update, but it only changed the commit message.

@russellb russellb force-pushed the empty-dataset-exception branch from b812fa4 to 9009a64 Compare July 13, 2024 07:16
@mergify mergify bot added the ci-failure label Jul 13, 2024
If at any point generate() on a block returns an empty dataset, this
is a failure condition. Go ahead and raise an exception right away in
that case.

This change was originally a subset of this commit: aakankshaduggal@256335e

Co-authored-by: shiv <[email protected]>
Co-authored-by: Aakanksha Duggal <[email protected]>
Co-authored-by: Kai Xu <[email protected]>
Signed-off-by: Russell Bryant <[email protected]>
@russellb russellb force-pushed the empty-dataset-exception branch from 9009a64 to 8f03a1f Compare July 13, 2024 07:33
@mergify mergify bot added testing Relates to testing and removed ci-failure labels Jul 13, 2024
@@ -28,6 +28,7 @@ def _noop_generate(self, samples, **gen_kwargs):
@patch.object(SamplePopulatorBlock, "generate", _noop_generate)
@patch.object(SelectorBlock, "generate", _noop_generate)
@patch("instructlab.sdg.llmblock.server_supports_batched", lambda c, m: True)
@patch.object(Pipeline, "_drop_duplicates", lambda self, dataset, cols: dataset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is getting uglier and uglier 🫣

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, can't say I felt proud about adding this line 😆

@markmc
Copy link
Contributor

markmc commented Jul 13, 2024

Looks good to me. I would merge it since the change has passed through Shiv, Aakanksha, Kai, and Russell.

It's not clear to me I can merge though?

Merging is blocked
Merging can be performed automatically with 2 approving reviews.

And the "merge pull request" button is disabled. Not clear to me I can override that?

@russellb
Copy link
Member Author

Looks good to me. I would merge it since the change has passed through Shiv, Aakanksha, Kai, and Russell.

It's not clear to me I can merge though?

Merging is blocked
Merging can be performed automatically with 2 approving reviews.

And the "merge pull request" button is disabled. Not clear to me I can override that?

I have a checkbox to override it. Maybe it’s because I’m a GitHub org owner? Either way I’ll change the repo settings for now.

image

@russellb russellb merged commit e636680 into instructlab:main Jul 13, 2024
11 checks passed
jwm4 pushed a commit to jwm4/sdg that referenced this pull request Dec 13, 2024
…_actions/rojopolis/spellcheck-github-actions-0.41.0

Bump rojopolis/spellcheck-github-actions from 0.40.0 to 0.41.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants