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

Adjust "make notebooks" command to fail when an external command fails #289

Open
fedarko opened this issue Mar 10, 2020 · 2 comments
Open
Labels
administrative Logistical matters that don't have much or anything to do with code backburner Low-priority things that are still good to keep track of bug Something isn't working

Comments

@fedarko
Copy link
Collaborator

fedarko commented Mar 10, 2020

Noticed that e.g. if you don't have DEICODE installed and you run make notebooks, the deicode_example notebook won't fail -- but if you check it out you'll see that the qiime deicode auto-rpca command failed because deicode wasn't installed, etc. This is obviously bad -- we want to make sure we're not publishing broken notebooks, so making us check everything automatically kinda defeats the purpose of make notebooks.

@fedarko fedarko added bug Something isn't working administrative Logistical matters that don't have much or anything to do with code backburner Low-priority things that are still good to keep track of labels Mar 10, 2020
@gibsramen
Copy link
Collaborator

gibsramen commented Mar 10, 2020

Had a similar problem when testing the ALDEx2 notebook with the bash commands errant exit codes not propagating to the notebook. I think it's simpler and easier if we just add assert statements after the standalone calls to make sure the output files exist. If the assert command fails then that should be a quick way to tell that something's wrong.

That being said...

The problem with the above approach is that it doesn't tell you what's wrong - just that the desired file doesn't exist. An alternative I played with was calling bash commands through the subprocess module which can capture STDOUT and STDERR and just raising an error with the output as custom text.

@fedarko
Copy link
Collaborator Author

fedarko commented Mar 10, 2020

Thanks!! Sorry, I should've remembered that you mentioned dealing with this sorta problem in the ALDEx2 tutorial before. Yeesh, this seems like the sort of thing that shouldn't be this broken.

From ipython/ipython#110 (comment), it might be possible for us to retrieve the exit code from Python -- but that may well end up being more complicated than the subprocess method you mentioned. If we can encapsulate this sort of check in a utility function, that might work -- as long as it isn't too intrusive to a user reading through the notebook.

The challenge is that we want to show off using the QIIME 2 command-line interface versions of these commands (since like 90% of users will be using things that way), hence our use of !qiime deicode cool-stuff ...... -- but the more infrastructure we pile around these commands, the more we risk scaring off new users, I guess.

For now I don't think this is a super urgent problem since obviously the notebooks work, and there aren't a ton of them... if we add more in the future it may be worth investigating this further

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
administrative Logistical matters that don't have much or anything to do with code backburner Low-priority things that are still good to keep track of bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants