ranaroussi / yfinance

Download market data from Yahoo! Finance's API
https://aroussi.com/post/python-yahoo-finance
Apache License 2.0
14.82k stars 2.44k forks source link

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

Open cmpunches opened 9 months ago

cmpunches commented 9 months ago

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:

https://github.com/ranaroussi/yfinance/blob/1d31e7ca016768c5a3f820c29049d3aa25132b7d/yfinance/base.py#L1687

I think these might help clarify the reasoning here:

cmpunches commented 9 months ago

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

https://github.com/ranaroussi/yfinance/issues/1378

dmoklaf commented 9 months ago

Ouch 3 points:

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 commented 9 months ago

Some context on why exceptions were redirected to logger:

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

dmoklaf commented 9 months ago

That’s great @ValueRaider thx

cmpunches commented 3 months ago

I completely forgot to follow up on this:

After reading these responses to what is clarification on a well understood best practice in library behaviour in February, I abandoned this library entirely and migrated to Alpha Vantage and have had no issues since.

I would suggest to any project maintainers that consume this library to consider doing the same or rewriting this project from scratch so that the clone can be used for serious projects without this type of issue, and without this type of reaction to reports of it.

Hijacking STDERR and STDOUT from a consumed library as default behaviour is just completely unacceptable, in a way obvious to pretty much anyone, and the idea that maintainers on a project this visible don't know that isn't inspiring of future confidence sufficient to trust this project in my own code.

I guess this issue can be closed, I just wanted to leave a breadcrumb for any others that need to do more mature things with their code than write a proof of concept.

ValueRaider commented 3 months ago

No need for that immaturity. I simply provided context.