Thanks for the pull request! Are there valid use-cases where a user expects an empty dataset from a leaf node? In other words, are we saving the user trouble by logging but ignoring any empty datasets? Or are we potentially masking a problem that they need to fix and re-run the entire generation? This might be the right way to address this, but I'm just trying to think through the user workflow here, how the user knows they have a problem, how they fix the problem, and whether a fatal error or just a warning log message makes that easier to spot and fix.

If it is appropriate to just log a message, perhaps we should also keep track of any taxonomy leaf nodes that resulted in empty datasets and log a final warning message at the very end of the generate run summarizing those? The single log message from each leaf node may be easy to miss during the generation loop if multiple leaf nodes are involved.

I think logging a summary at the end is a great idea!

I do believe it’s valid for if a user is generating sdg against a full taxonomy (using the taxonomy-base empty parameter) that one leaf node out of 140 for example (that is about the number of leaf nodes in the community taxonomy) shouldn’t cause a complete failure of the sdg run.

Your example of the entire community taxonomy is a good one, and I agree that even if we can't generate data for all leaf nodes, generating what we can and logging the leaf nodes that failed is better than aborting entirely after multiple hours of generation.

/hold

ready for review but want to do an e2e test before merging.

I was going to ask you to please add a unit test in test_generate_data.py to verify that when a mocked generate block returns an empty dataset that the generation still runs to completion instead of erroring out. However, the code in generate_data.py needs a bit of refactoring to make that easy, and until that's done it requires quite a bit of mocking out things in generate_data to do that test. There are a few examples of this in test_generate_data.py if you want to take a stab at that, but if not this looks reasonable to merge as-is and we can create a separate issue to do some code cleanup and refactoring to make it easier to write new unit tests for the main generation loop.

Ben: Thank you so much for taking the time to illustrate that unit test example: I will note that general pattern for future PRs as well. These changes look great and I approve!!!!

/unhold

testing complete

@instructlab/sdg-maintainers do we think this is a potential candidate to merge? It seems like it is impacting some client workflows when they are utilizing generation against the full community taxonomy