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

Fixed currency fetching for ranges > 30 days #479

Merged
merged 4 commits into from Feb 6, 2017

Conversation

Projects
None yet
2 participants
@lenlo
Contributor

lenlo commented Jan 29, 2017

Old currency rates would not be retrieved outside of a 30 day window. You can reproduce this by clearing out currencies.db and opening a moneyGuru file with old transactions in it. Note that the currency rate will stay the same for anything older than 30 days ago. Fixed by queueing up requests for the whole time period in 30 day chunks.

lenlo added some commits Jan 11, 2017

Fixed currency fetching for ranges > 30 days
The code that downloads historical currencies would limit ranges larger than
30 days to only the last 30 days of that range to avoid "various problems,
such as hangs." Unfortunately, this meant that the rest of the range never
got fetched. Fixed by splitting the range into 30-day chunks and queueing
them all up.

Also forced all currency codes to be uppercase when looking them up for an
unrelated issue.
Fixed typo in RatesDB's ensure_rates...
...that would ignore the previously cached dates of a rate when computing
which new ones to fetch (start_date => range_start).

@hsoft hsoft self-requested a review Jan 29, 2017

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Jan 29, 2017

Owner

With an unrestrained while loop, I fear edge cases where moneyGuru ends up making very many requests that cause undue load on the client or server. If I recall correctly, the currency system works on a "need to know" basis. Whenever it needs a rate that it doesn't have, it requests it, using a range if needed (with a maximum of 30 days). Because the request is async, the first time it asks for this rate, it's going to be a wrong one, but the next time you ask for it, it's going to be good.

Sure, it causes a bit of inaccuracies on a fresh moneyGuru instance using a file with old transactions, but these end up fixing themselves up with time and I always though it was good enough. What do you think?

Owner

hsoft commented Jan 29, 2017

With an unrestrained while loop, I fear edge cases where moneyGuru ends up making very many requests that cause undue load on the client or server. If I recall correctly, the currency system works on a "need to know" basis. Whenever it needs a rate that it doesn't have, it requests it, using a range if needed (with a maximum of 30 days). Because the request is async, the first time it asks for this rate, it's going to be a wrong one, but the next time you ask for it, it's going to be good.

Sure, it causes a bit of inaccuracies on a fresh moneyGuru instance using a file with old transactions, but these end up fixing themselves up with time and I always though it was good enough. What do you think?

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Jan 31, 2017

Contributor

You might want to take a look at the currency code again. As far as I can tell, ensure_rates will currently only download at most 30 days worth of rates for any currency during any single moneyGuru run. If asked to download e.g. all USD/CAD exchange rates from 2016-01-01 to 2017-01-30, it will queue up a request for 2017-01-01 to 2017-01-30 but then mark the whole 2016-01-01--2017-01-30 range as already fetched. This means the next time it's called, it will return without queueing up any new downloads up as it believes the whole range has been downloaded.

You can fix this by replacing:
self._fetched_ranges[currency] = (start_date, date.today())
with:
self._fetched_ranges[currency] = (range_start, date.today())

Now, it will still only download the most recent 30 days on the first call, but on the second call, it will download the next 30 most recent days, etc. I'm guessing that that might be what you intended.

I'm still not convinced that this is sufficient, though, as ensure_rates only is called at load time or when a transaction's date and amount are changed at the same time -- and then only for that transaction's date forward. This means that it can take a long time (if ever) for moneyGuru to have the correct exchange rates for transactions dates in the past.

Here's a simple example: Suppose you start off with an empty currency database and that you open up a moneyGuru file that has a default currency of USD and only contains one account with 1000 EUR as of 2016-01-01: EUR1000.moneyguru.zip

The moment you open the file, you will trigger the download of rates for the last 30 days (January 2017 at today's date). As the base currency is USD, this will be reflected on the Net Worth sheet with a lot of fluctuations in the net worth for January 2017, but completely flat for the whole year of 2016.

This won't change and no new rates will be downloaded until you either add a new transaction in the past or modify an old transaction's date and amount, and then only for the next block of the 30 most recent days after that transaction's date. Not only that, but you also have to close and reopen the sheet to see the changes as it doesn't fresh automatically. I don't know about you, but I'm not really happy with this.

My change makes sure that the whole requested interval does get downloaded, but still only in 30 day chunks. This means at most 12 requests per year/currency gets sent out, which does not seem particularly excessive to me. There's still no automatic refresh of open sheets, but I thought I'd look to see if I couldn't add some kind of "listener" support that will notify the UI in the main thread whenever new rates have come in.

What do you think?

Contributor

lenlo commented Jan 31, 2017

You might want to take a look at the currency code again. As far as I can tell, ensure_rates will currently only download at most 30 days worth of rates for any currency during any single moneyGuru run. If asked to download e.g. all USD/CAD exchange rates from 2016-01-01 to 2017-01-30, it will queue up a request for 2017-01-01 to 2017-01-30 but then mark the whole 2016-01-01--2017-01-30 range as already fetched. This means the next time it's called, it will return without queueing up any new downloads up as it believes the whole range has been downloaded.

You can fix this by replacing:
self._fetched_ranges[currency] = (start_date, date.today())
with:
self._fetched_ranges[currency] = (range_start, date.today())

Now, it will still only download the most recent 30 days on the first call, but on the second call, it will download the next 30 most recent days, etc. I'm guessing that that might be what you intended.

I'm still not convinced that this is sufficient, though, as ensure_rates only is called at load time or when a transaction's date and amount are changed at the same time -- and then only for that transaction's date forward. This means that it can take a long time (if ever) for moneyGuru to have the correct exchange rates for transactions dates in the past.

Here's a simple example: Suppose you start off with an empty currency database and that you open up a moneyGuru file that has a default currency of USD and only contains one account with 1000 EUR as of 2016-01-01: EUR1000.moneyguru.zip

The moment you open the file, you will trigger the download of rates for the last 30 days (January 2017 at today's date). As the base currency is USD, this will be reflected on the Net Worth sheet with a lot of fluctuations in the net worth for January 2017, but completely flat for the whole year of 2016.

This won't change and no new rates will be downloaded until you either add a new transaction in the past or modify an old transaction's date and amount, and then only for the next block of the 30 most recent days after that transaction's date. Not only that, but you also have to close and reopen the sheet to see the changes as it doesn't fresh automatically. I don't know about you, but I'm not really happy with this.

My change makes sure that the whole requested interval does get downloaded, but still only in 30 day chunks. This means at most 12 requests per year/currency gets sent out, which does not seem particularly excessive to me. There's still no automatic refresh of open sheets, but I thought I'd look to see if I couldn't add some kind of "listener" support that will notify the UI in the main thread whenever new rates have come in.

What do you think?

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Feb 4, 2017

Owner

You're right. The whole system as it is is awkward.

... but what about a sanity check? For example, we don't queue more than 5 years at once? Otherwise, a mistyped year (1900) could cause a lot of requests.

Owner

hsoft commented Feb 4, 2017

You're right. The whole system as it is is awkward.

... but what about a sanity check? For example, we don't queue more than 5 years at once? Otherwise, a mistyped year (1900) could cause a lot of requests.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Feb 5, 2017

Contributor

Hmm. Aren't fetches already bounded by the currency's start_date (& end_date)? If not, they probably should be. Also, I assume that the currency fetchers won't have any date before a certain date and arguably ought to filter out requests that they won't be able to satisfy anyway. I'll can check it out...

Contributor

lenlo commented Feb 5, 2017

Hmm. Aren't fetches already bounded by the currency's start_date (& end_date)? If not, they probably should be. Also, I assume that the currency fetchers won't have any date before a certain date and arguably ought to filter out requests that they won't be able to satisfy anyway. I'll can check it out...

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Feb 5, 2017

Owner

Yes, you're right (it's been a while since I've played in that code). We can see it at https://github.com/hsoft/moneyguru/blob/master/core/model/currency.py#L136

That would make this feature merge-able as-is. The only problem is that your change makes tests fail. So it's only a matter of making tests pass and we're good to go.

Owner

hsoft commented Feb 5, 2017

Yes, you're right (it's been a while since I've played in that code). We can see it at https://github.com/hsoft/moneyguru/blob/master/core/model/currency.py#L136

That would make this feature merge-able as-is. The only problem is that your change makes tests fail. So it's only a matter of making tests pass and we're good to go.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Feb 5, 2017

Contributor

Fixed tests.

Contributor

lenlo commented Feb 5, 2017

Fixed tests.

@hsoft

hsoft approved these changes Feb 6, 2017

looks good, works well, thanks!

@hsoft hsoft merged commit c409587 into hsoft:master Feb 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment