Skip to content

Give a pragmatic solution to suppressing noisy output - #153

Merged
Carreau merged 5 commits into
jupyterlite:mainfrom
steppi:fix-bad-release-with-hack
Mar 22, 2024
Merged

Give a pragmatic solution to suppressing noisy output#153
Carreau merged 5 commits into
jupyterlite:mainfrom
steppi:fix-bad-release-with-hack

Conversation

@steppi

@steppi steppi commented Mar 20, 2024

Copy link
Copy Markdown
Collaborator

This PR replaces the broken means of suppressing noisy output from #150 with a pragmatic hack. (Note that although @Carreau submitted #150, I was the one who came up with this broken solution, and he was only implementing my idea).

The pragmatic hack I've made is to offer an option jupyterlite_silence in config.py. Setting this to True causes the stderr and stout from the call to jupyterlite build to be directed to dev/null. I've verified that this works for suppressing output, and that the notebooks still work. I have not made jupyterlite_silence=True the default because it's a drastic step which suppresses even warnings and errors.

I think it's reasonable to go with this hack, at least for the time being, until the necessary changes can be made in jupyterlite removing unguarded print statements and making doit for tasks configurable.

Many apologies for jumping the gun and making a broken release. I will be sure to learn my lesson from this. I will need help revoking the bad release from PyPI.

cc @Carreau, @martinRenou, @jtpio @matthewfeickert

@matthewfeickert

Copy link
Copy Markdown

Thanks very much for being so fast and responsive on this @steppi! Very much appreciated. 🙏

Many apologies for jumping the gun and making a broken release. I will be sure to learn my lesson from this.

I think we've all been there before, so no worries. :)

if app.env.config.jupyterlite_silence:
kwargs["stdout"] = subprocess.DEVNULL
kwargs["stderr"] = subprocess.DEVNULL
subprocess.run(command, **kwargs)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should check the recur code of subprocess run (CompletedProcess(args=..., returncode=1=...)) and fail if it is Non-zero (i'll check).

I also think that we might want to have stdout/err be PIPE and print them anyway if the return code is non-zero

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good idea.

@martinRenou

Copy link
Copy Markdown
Member

Many apologies for jumping the gun and making a broken release

No worries! I stopped counting the number of broken releases I made 🙈

@steppi

steppi commented Mar 20, 2024

Copy link
Copy Markdown
Collaborator Author

I've made it so stdout and stderr of jupyterlite build is still printed if the command fails following @Carreau's suggestion. I've also made jupyterlite_silence = True the default, following the suggestion of @rgommers here: scipy/scipy#20277 (comment).

@steppi

steppi commented Mar 20, 2024

Copy link
Copy Markdown
Collaborator Author

Many apologies for jumping the gun and making a broken release

No worries! I stopped counting the number of broken releases I made 🙈

Thanks. That makes me feel better.

@steppi

steppi commented Mar 21, 2024

Copy link
Copy Markdown
Collaborator Author

@Carreau, @martinRenou, @jtpio. Can we merge this and make a release? It would fix a broken build in one of SciPy’s CI jobs.

@Carreau

Carreau commented Mar 22, 2024

Copy link
Copy Markdown
Collaborator

Ok, let's merge this and try. It's ok to do broken releases, it happens.

@Carreau
Carreau merged commit 1e80dd1 into jupyterlite:main Mar 22, 2024
@steppi

steppi commented Mar 22, 2024

Copy link
Copy Markdown
Collaborator Author

Thanks @Carreau!

@jtpio

jtpio commented Mar 22, 2024

Copy link
Copy Markdown
Member

Yes, thanks all for working on this!

@Carreau Carreau mentioned this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants