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

Use a single warning interface. #4593

Open
mtessmer opened this issue May 9, 2024 · 5 comments
Open

Use a single warning interface. #4593

mtessmer opened this issue May 9, 2024 · 5 comments

Comments

@mtessmer
Copy link

mtessmer commented May 9, 2024

Is your feature request related to a problem?

Currently, MDAnlysis is using both the logging module and the warnings module to warn users, even within the same file. e.g. MDAnalysis/coordinates/TRJ.py uses logging on line 151 and warnings on line 280. This makes it difficult for users to interact with warnings in a singular way.

Describe the solution you'd like

A single warning interface, preferably logging, would allow users better control when handling warning messages.

Describe alternatives you've considered

Alternatively, MDA could provide it's own methods for handling warnings, however, that would likely be a lot more work.

Additional context

@orbeckst
Copy link
Member

orbeckst commented May 9, 2024

What is the specific problem that you're facing? Is it about seeing the same message twice or filtering messages?

I agree that it's not super-elegant to have both warnings and logging but it is nice to have a programmatic "exception-like" thing happening with warnings while having a lot flexibility with logging.

Some background: Most people do not enable logging, especially not beginners (few people probably know about mda.start_logging()) so the only way to start getting a warning to users is to use warnings.warn().

Logging hasn't been enabled by default because we had considered this the domain of more experiences users who would also use logging in their own scripts/programs. Our default logger also creates a file (mdanalysis.log) and we don't want to write files unless the user asks for it.

Are you suggesting that we enable logging to console by default (while keeping logging to file optional) and get rid of warnings?

@tylerjereddy
Copy link
Member

(few people probably know about mda.start_logging())

Interesting, I didn't know about it either.

Are you suggesting that we enable logging to console by default (while keeping logging to file optional) and get rid of warnings?

That sounds tricky at first consideration, couldn't downstream/end users depend on the usual (i.e., pytest warn->error) approaches to erroring out on deprecation warnings to detect/update their code as we develop?

@mtessmer
Copy link
Author

mtessmer commented May 9, 2024

I use MDAnalysis as a dependency of my own Python package. My package uses a lot of functions that both log and throw warnings. I often share scripts with users and collaborators via google colab. Interestingly, google colab seems to display logging warnings even if you don't enable via mda.start_logging(). This results in shared scripts that are littered with MDA warnings that are irrelevant to the function of the script. Currently, I have solved this by manually suppressing warnings.warn calls at several places in my code base and managing logging warnings separately. It would be nice to manage warnings via a single consistent interface so each use of mda that throws warnings does not need to be handled individually.

Furthermore, if using warnings.warn is how you get warnings to the user, then they probably aren't being logged when logging is enabled. This of course would result in an incomplete logging history.

@IAlibay
Copy link
Member

IAlibay commented May 9, 2024

I feel like someone at some time gave me a pretty good overview of they process for doing warnings and it was great - I'll have a dig around for if. I do think a review of when we use UserWarnings should be done.

My longer ramble:

DeprecationWarning are clean and simple - they should be noisy and annoying because they shouldn't be hit downstream if you can avoid it.

UserWarnings probably shouldn't be invoked every time you have something that could be expected but just doesn't follow convention - systems without elements or similar imho should fall under this, if you just expect folks to manually guess after the fact then are you just telling them about their system or are you warning them?

I.e. if I'm catching a lack of elements I'm try/excepting on an empty elements attribute, not a UserWarning.

@orbeckst
Copy link
Member

orbeckst commented May 9, 2024

FYI: For another project I got convinced to switch to loguru to replace logging.

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

No branches or pull requests

4 participants