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

mephisto check should output to stderr if it fails #916

Open
mjkanji opened this issue Oct 12, 2022 · 4 comments
Open

mephisto check should output to stderr if it fails #916

mjkanji opened this issue Oct 12, 2022 · 4 comments

Comments

@mjkanji
Copy link
Contributor

mjkanji commented Oct 12, 2022

The mephisto check command currently uses a print statement and click.echo() when it fails. This results in silent errors which may not be picked up by a CI/CD tool.

For example, in this run of the Docker Testing Matrix job, the 'Check that Mephisto was installed correctly' stage passes, but if you look at the details, mephisto check actually fails:

Something went wrong.
The provided Mephisto data directory /mephisto/data as set in /root/.mephisto/config.yml is not a directory! Please locate your Mephisto data directory and update /root/.mephisto/config.yml to point to it.

@JackUrb
Copy link
Contributor

JackUrb commented Oct 12, 2022

Hi @mjkanji - this is a good point, much of Mephisto hasn't been built with this particular setting in mind, but it would definitely be an improvement for us to do. I think it'd be appropriate at least here (and eventually in other scripts utilities) to switch to print(<>, file=sys.stderr). Happy to review any solution you'd want to put up though!

@mjkanji
Copy link
Contributor Author

mjkanji commented Oct 13, 2022

@JackUrb That sounds good to me! Though, I'm not sure I understand why one message use rich.print and the other click.echo. I don't have much experience with click/rich_click so if there's a very obvious reason I'm missing, let me know!

print("\n[red]Something went wrong.[/red]")
click.echo(e)

From my cursory look at rich-click, it seems that it doesn't give you the flexibility to route to stderr so would you be okay with my changing both calls to rich.print and then using file=sys.stderr as you suggested to route the message correctly?

@JackUrb
Copy link
Contributor

JackUrb commented Oct 13, 2022

@pringshia do you have any intuition as to why some of the cli uses click.echo and why some uses rich.print? I imagine the prior echo calls are just old and all can be updated to rich.print for consistency.

@pringshia
Copy link
Contributor

Yes, rich support came later, so not all of the codebase was transitioned to it. Feel free to change the click.echo references to rich.print

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants