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

This library uses STDERR instead of throwing exceptions when failures are encountered #1863

Open
cmpunches opened this issue Feb 18, 2024 · 4 comments

Comments

@cmpunches
Copy link

cmpunches commented Feb 18, 2024

Describe Design Problem

This issue seems to be pervasive through the library but can be demonstrated with the below (I use pandas_datareader which wraps this library):

import pandas_datareader.data as web

fetch = web.get_data_yahoo( "AACI", 1577854800, 1609390800)

It'll return successfully while producing the following output:

1 Failed download:
['AACI']: Exception("%ticker%: Data doesn't exist for startDate = 1577854800, endDate = 1609390800")
[*********************100%%**********************]  1 of 1 completed

The error output is printed to STDERR instead of yfinance throwing an exception for the consumer of the yfinance library to handle. This breaks error handling and pollutes my application's output.

Essentially this means that yfinance currently hijacks STDERR for consuming applications so that it can report errors instead of throwing an exception with the same information (using exceptions allows consuming applications to determine how failures are handled or represented).

A library should almost never write to stdout and stderr for errors -- this also has very big implications for flow control and error handling in consuming logic-- whether by an application that uses this library or another library that wraps it (such as pandas_datareader which is a very visible example).

It seems to be littered all through the library, but this code specifically exemplifies the issue:

logger.error(f"Failed to get ticker '{self.ticker}' reason: {e}")

I think these might help clarify the reasoning here:

  • the errors that libraries throw are sometimes fatal to the logic of the consuming application and so need to be handled in that logic, which will expect an exception from the called function in a library to catch
  • In most cases, an application uses stdout+stderr for its own purposes, at its own level of abstraction to talk to the user. STDOUT and STDERR are categorically for the application to talk, not its libraries. For example, this current STDERR behaviour will completely break CURSES-based user interfaces.
  • An exception notifies a consuming developer of a problem, output notifies the user. The user is often abstracted very far away from the library code so this is an essential practice to incorporate and it will have a huge impact on other projects to do this. It's good to log errors, but also important to defer that function to whatever the consumer is using.
  • An exception can have the same data as what you're currently sending to logger, it just lets the application handle how or whether to show that to the user, or decide if it can even continue whatever its trying to do -- and that application may be only using your library for something low level in one piece of it that isn't related to the core function of the application that the developer will care about but that the user won't.
@cmpunches
Copy link
Author

cmpunches commented Feb 18, 2024

This problem appears to have always been there, but was exacerbated by the introduction of the use of logger:

#1378

@dmoklaf
Copy link

dmoklaf commented Feb 18, 2024

Ouch 3 points:

  • This is true that errors shall be raised as exceptions when thy happen synchronously within a call of the API client (which is the case of the entire yfinance library, no background task doing things on the side)
  • I disagree that replacing prints by logs exacerbates this - if the stderr stream needs to be preserved (eg because there is a terminal UI deployed), then configure the logs to not use it
  • My previous issue Library using print rather than logger #1378 was aimed at INFO-level and WARNING-level prints, not errors

So logs should be kept (if their content is useful to the library's author or to the library's user, I have no opinion there as I use yfinance sporadically only), but below ERROR-level only, and errors shall be raised as exceptions indeed

@ValueRaider
Copy link
Collaborator

ValueRaider commented Feb 18, 2024

Some context on why exceptions were redirected to logger:

  • users didn't want failed fetches to abort their analysis loops
  • yf.download() parallel download. Suppose one symbol suddenly delisted, that would prevent returning other symbols' good data.

Argument raise_errors was added to history() to optionally restore exception raising.

@dmoklaf
Copy link

dmoklaf commented Feb 18, 2024

That’s great @ValueRaider thx

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

No branches or pull requests

3 participants