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

Add some useful debug prints #1375

Closed

Conversation

flynneva
Copy link

@flynneva flynneva commented Aug 11, 2023

  • Changed a bunch of the code comments to debug prints instead to help when debugging

Relates to #1374

@waylan
Copy link
Member

waylan commented Aug 11, 2023

Not sure how this helps.

@waylan waylan added the needs-decision A decision needs to be made regarding request. label Aug 11, 2023
@flynneva flynneva force-pushed the improve-large-table-builds branch from e819187 to b748e8f Compare August 11, 2023 17:54
@waylan
Copy link
Member

waylan commented Aug 11, 2023

Still not sure how this helps. In what workflow would I use this?

@waylan
Copy link
Member

waylan commented Aug 11, 2023

Now that I have looked at #1374, I realize that you were using MkDocs and the debug messages were being printed by MkDocs logger. That is not the correct way to debug something like that would be to call Markdown directly passing in your sample Markdown source. And there are existing Python tools which can help to isolate which part of the code is taking all the time. Therefore, I am closing this as wontfix.

@waylan waylan closed this Aug 11, 2023
@waylan waylan added wontfix The issue will not be fixed for the stated reasons. rejected The pull request is rejected for the stated reasons. and removed needs-decision A decision needs to be made regarding request. labels Aug 11, 2023
@flynneva
Copy link
Author

@waylan confused by your comment here about "using the mkdocs logger". The changes in this PR use the MARKDOWN logged already used in this file. The prints I copied to the issue were just formatted in a different way using the typical python logging API. No relation to mkdocs at all.

@waylan
Copy link
Member

waylan commented Aug 14, 2023

Yes, you were using the Markdown logger, but from MkDocs', which is the point. You should have separated out the Markdown call and tested that individually. And using the method you used is like stabbing in the dark until you maybe hit your target. And in fact you did not hit the target. You hit the wrapper around your target and many others, which didn't really help. It was my guess based on years of experience dealing with these sorts of reports that narrowed down the issue.

In comparison, look at the discussion for #161, specifically this comment. With one short line of code I narrowed it down to the one method which was taking long. And then I was able to quickly build a test which narrowed the issue down further. The first line of code isn't provided, but I did mention cProfile, which is in the Python Standard Library. I'm assuming I used some variation of cProfile.run(markdown.markdown(some_input)). That is a much quicker and more precise way of finding out what is causing a slowdown.

@flynneva
Copy link
Author

@waylan not really sure what you are saying here in relation to this PR. All it does is adds some debug prints....nothing more.

If you don't think it's helpful, that's ok with me 👍

@waylan
Copy link
Member

waylan commented Aug 15, 2023

I meant to mention this before, but we already have users requesting less verbose output (see #954), so it would be worse to add more from their perspective. As there are other ways to debug the issue you had, then the best approach is to use those other ways, not to add more debug messages.

@flynneva
Copy link
Author

@waylan thats fine, and totally get it. Just that's a very different answer than what you said at first. I'll remove the debug prints from my other PR then, no worries! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected The pull request is rejected for the stated reasons. wontfix The issue will not be fixed for the stated reasons.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants