MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.26k stars 641 forks source link

Use a single warning interface. #4593

Open mtessmer opened 1 month ago

mtessmer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

(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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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