Skip to content

Write table report progress messages to stderr#1236

Merged
jeromedockes merged 8 commits into
skrub-data:mainfrom
priscilla-b:write-to-stderr
Feb 11, 2025
Merged

Write table report progress messages to stderr#1236
jeromedockes merged 8 commits into
skrub-data:mainfrom
priscilla-b:write-to-stderr

Conversation

@priscilla-b

Copy link
Copy Markdown
Contributor

Towards #1175

@priscilla-b priscilla-b changed the title Write to table report progress messages to stderr Write table report progress messages to stderr Feb 9, 2025

@Vincent-Maladiere Vincent-Maladiere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello @priscilla-b, thank you for this PR, LGTM :)

@Vincent-Maladiere Vincent-Maladiere dismissed their stale review February 10, 2025 11:40

Woops, actually, need to look at what is failing in the CI

@priscilla-b

Copy link
Copy Markdown
Contributor Author

Woops, actually, need to look at what is failing in the CI

Yeahh trying to figure that out @Vincent-Maladiere
Will add to the changelog, but can't figure out why the others are failing.

@Vincent-Maladiere

Vincent-Maladiere commented Feb 10, 2025

Copy link
Copy Markdown
Member

So the issue comes from doctest, which checks that examples in the docstring match their expected value.

We configured pytest to run doctest at the beginning of the test suite:

skrub/pyproject.toml

Lines 276 to 277 in 4cdd5d9

addopts = "--doctest-modules"
doctest_optionflags = "NORMALIZE_WHITESPACE ELLIPSIS"

You can reproduce this error with:

from skrub import TableReport
import doctest

doctest.run_docstring_examples(
    TableReport,
    globals(),
    optionflags=doctest.NORMALIZE_WHITESPACE + doctest.ELLIPSIS
)

And to fix it, you have to replace Processing ... with <BLANKLINE> in the docstring of skrub/_reporting/_table_report.py L86

Let me know if this works for you :)

@priscilla-b

priscilla-b commented Feb 10, 2025

Copy link
Copy Markdown
Contributor Author

So the issue comes from doctest, which checks that examples in the docstring match their expected value.

Yes, I was able to reproduce it !
And the fix works, thank you :)

@Vincent-Maladiere Vincent-Maladiere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great! This time, LGTM

@jeromedockes jeromedockes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @priscilla-b ! super useful 🚀 . a couple of small adjustments and we can merge it!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the second print to add a newline on line 103 should also go to stderr

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it, will work on that

assert capsys.readouterr().out != ""


def test_write_to_stderr(df_module, capsys):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

once all progress output goes to stderr I think you will need to change the test_verbosity_parameter above to replace out with err

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, that makes sense

@jeromedockes jeromedockes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 🎉 thanks a lot @priscilla-b 👍

@jeromedockes jeromedockes merged commit 60aad93 into skrub-data:main Feb 11, 2025
rcap107 pushed a commit to rcap107/skrub that referenced this pull request Apr 2, 2025
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

Successfully merging this pull request may close these issues.

3 participants