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

fix: assetalloc_class fail due to favapricemap vs beancount pricemap #92

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

aldur
Copy link
Contributor

@aldur aldur commented Apr 11, 2024

No description provided.

@redstreet
Copy link
Owner

Thank you, and the code seems to make sense, but strangely, I've not been able to reproduce this issue. Would you mind showing how I can repro it?

@aldur
Copy link
Contributor Author

aldur commented Apr 12, 2024

Yeah it wasn't an issue for me either initially, it started after I set the following:

2024-03-23 custom "fava-extension" "fava_investor" "{
  'asset_alloc_by_class' : {
      'accounts_patterns': ['Assets:.*'],
  }
}

Before setting that, what was happening was that I had income (-) /assets (+) transactions offsetting each other and thus showing up as net 0 on the balance sheet. Once I asked the allocation to only consider assets, I started running into this error.

If setting this doesn't trigger it for you too I'll try to come up with a minimum example -- not being able to share my full beancount index.

@redstreet
Copy link
Owner

I tried your config, and unfortunately couldn't get it to repro on my end. I imagine the scenario might have to do with the specifics of your journal, which as you say is always a pain to reduce to a minimal example while not including personal financial data.

I'm sure you are, but as a sanity check, are you using the latest version of fava and top of tree of fava_investor?

Your code change makes sense regardless, and should probably go in anyway, because convert is from beancount, and we should giving it the beancount price map. But I 'd like to understand where this bug manifests, which will provide confidence in ensuring additional problems are not still hiding.

If at all it's not too much trouble to reduce this to a minimal example, that would be great. Thank you!

@redstreet
Copy link
Owner

redstreet commented Apr 12, 2024

One other question: does the console output an error or other messages? Also, have you tried to run investor assetalloc-class on the command line?

@aldur
Copy link
Contributor Author

aldur commented Apr 12, 2024

I'm sure you are, but as a sanity check, are you using the latest version of fava and top of tree of fava_investor?

Confirmed, here are a few info:

poetry run pip freeze | grep fava
fava==1.27.3
fava_investor @ git+https://github.com/redstreet/fava_investor@f41b22c1c613b6cfab8dfea2a5434901223649c8

And the console log:

Exception on /adrianos-ledger/extension/Investor/ [GET]
Traceback (most recent call last):
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/flask/app.py", line 1455, in wsgi_app
    response = self.full_dispatch_request()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/flask/app.py", line 869, in full_dispatch_request
    rv = self.handle_user_exception(e)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/flask/app.py", line 867, in full_dispatch_request
    rv = self.dispatch_request()
         ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/flask/app.py", line 852, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/fava/application.py", line 344, in extension_report
    content = Markup(template.render(ledger=g.ledger, extension=ext))
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/fava_investor/templates/Investor.html", line 188, in top-level template code
    {% set results = extension.build_assetalloc_by_class() %}
    ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/fava_investor/__init__.py", line 21, in build_assetalloc_by_class
    return libassetalloc.assetalloc(accapi, self.config.get('asset_alloc_by_class', {}))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/fava_investor/modules/assetalloc_class/libassetalloc.py", line 206, in assetalloc
    asset_buckets = bucketize(balance, accapi)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/fava_investor/modules/assetalloc_class/libassetalloc.py", line 111, in bucketize
    amount = convert.convert_amount(pos.units, base_currency, price_map,
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/beancount/core/convert.py", line 202, in convert_amount
    _, rate = prices.get_price(price_map, base_quote, date)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/beancount/core/prices.py", line 356, in get_price
    return get_latest_price(price_map, base_quote)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/beancount/core/prices.py", line 329, in get_latest_price
    price_list = _lookup_price_and_inverse(price_map, base_quote)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aldur/Library/Caches/pypoetry/virtualenvs/aldurs-beans-m7UNDcR_-py3.12/lib/python3.12/site-packages/beancount/core/prices.py", line 278, in _lookup_price_and_inverse
    return price_map[base_quote]
           ~~~~~~~~~^^^^^^^^^^^^
TypeError: 'FavaPriceMap' object is not subscriptable

Running investor assetalloc-class from the command line works.

To get the output I expected (both on fava and from the cli) I have to apply the following patch:

diff --git a/fava_investor/modules/assetalloc_class/libassetalloc.py b/fava_investor/modules/assetalloc_class/libassetalloc.py
index acc668b..4e491e6 100644
--- a/fava_investor/modules/assetalloc_class/libassetalloc.py
+++ b/fava_investor/modules/assetalloc_class/libassetalloc.py
@@ -113,7 +113,8 @@ def bucketize(vbalance, accapi):
             if amount.currency != base_currency:
                 sys.stderr.write("Error: unable to convert {} to base currency {}"
                                  " (Missing price directive?)\n".format(pos, base_currency))
-                sys.exit(1)
+                # sys.exit(1)
+                continue

That's because I have some assets that are unlisted / unpriced.

I completely understand how a minimum example would give us better confidence, let me try to dig into that. It might take me a couple days, but I'll be back :)

@aldur
Copy link
Contributor Author

aldur commented Apr 12, 2024

Actually, that was quicker than expected.

Starting from the example on pythonanywhere, add the following to commodities:

2022-08-07 commodity VMMXY
  asset_allocation_stock_domestic_sector: 100
  a__quoteType: "MONEYMARKET"
  name: "Vanguard Money Market Reserves - Vanguard Cash Reserves Federal Money Market Fund"

Then on the index add:

2022-08-07 commodity VMMXY
  asset_allocation_stock_domestic_sector: 100
  a__quoteType: "MONEYMARKET"
  name: "Vanguard Money Market Reserves - Vanguard Cash Reserves Federal Money Market Fund"

2015-01-05 * "Investing 40% of cash in VBMPX"
  Assets:US:Vanguard:VMMXY                          3.513 VMMXY {136.65 USD, 2015-01-05}
  Assets:US:Vanguard:Cash                         -480.05 USD

That should be enough to trigger the issue.

PS: Sorry, had little time so left the example a little dirty :)

@aldur
Copy link
Contributor Author

aldur commented Apr 24, 2024

Sorry to bump this, is the example good enough? :)

@redstreet redstreet merged commit c202048 into redstreet:main Apr 24, 2024
3 checks passed
@redstreet
Copy link
Owner

My apologies, this got lost in my inbox, so thanks for the bump!

Indeed, the example repros, and issue is confirmed. Merged. Thank you for filing the issue and the fix, much appreciated!

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

Successfully merging this pull request may close these issues.

2 participants