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

Try to update calc on init #92

Merged
merged 88 commits into from
Feb 9, 2024
Merged

Try to update calc on init #92

merged 88 commits into from
Feb 9, 2024

Conversation

jobrachem
Copy link
Contributor

This is my proposal for #79

In this PR, a calculator will try to update its value during initialization. If the update fails, a warning with the title of the error is logged. Also, the full traceback of the error is logged to the debug logger.

Example 1:

>>> import liesel.model as lsl
>>> 
>>> a = lsl.Data(1.0)
>>> b = lsl.Calc(lambda x: x / 0, a)
liesel.model.nodes - WARNING - Calc(name="") was not updated during initialization, because the following exception occured: RuntimeError('Error while updating Calc(name="").'). See debug log for the full traceback.

Example 2:

>>> import logging
>>> import liesel.model as lsl
>>> 
>>> logger = logging.getLogger("liesel")
>>> logger.handlers[0].setLevel(logging.DEBUG)
>>> 
>>> a = lsl.Data(1.0)
>>> b = lsl.Calc(lambda x: x / 0, a)
liesel.model.nodes - WARNING - Calc(name="") was not updated during initialization, because the following exception occured: RuntimeError('Error while updating Calc(name="").'). See debug log for the full traceback.
liesel.model.nodes - DEBUG - Calc(name="") was not updated during initialization, because the following exception occured:
Traceback (most recent call last):
  File "/Users/johannesbrachem/Documents/git/liesel/liesel/model/nodes.py", line 526, in update
    self._value = self.function(*args, **kwargs)
  File "<string>", line 1, in <lambda>
ZeroDivisionError: float division by zero

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/johannesbrachem/Documents/git/liesel/liesel/model/nodes.py", line 500, in __init__
    self.update()
  File "/Users/johannesbrachem/Documents/git/liesel/liesel/model/nodes.py", line 528, in update
    raise RuntimeError(f"Error while updating {self}.") from e
RuntimeError: Error while updating Calc(name="").

@jobrachem jobrachem added the enhancement New feature or request label Sep 7, 2023
@jobrachem jobrachem requested a review from wiep September 7, 2023 12:22
@jobrachem jobrachem self-assigned this Sep 7, 2023
@jobrachem
Copy link
Contributor Author

@wiep this is something we discussed briefly during the weekly on August 30. You had some reservations about the general idea to try to update a calculator on initialization - so to enrich the conversation about this proposal, I created this PR which shows how I think the update could be implemented without breaking the calculator's ability to be initialized without a working update. What do you think?

Due to the exception being caught and re-raised in the update() method, we do not get the actual error text here.
@jobrachem jobrachem linked an issue Sep 7, 2023 that may be closed by this pull request
@jobrachem jobrachem marked this pull request as draft September 11, 2023 09:49
@jobrachem
Copy link
Contributor Author

Converted back to draft, because I just saw that with this implementation, we get the following warnings during model initialization:

liesel.model.nodes - WARNING - Calc(name="_model_log_prior") was not updated during initialization, because the following exception occured: RuntimeError('Error while updating Calc(name="_model_log_prior").'). See debug log for the full traceback.
liesel.model.nodes - WARNING - Calc(name="_model_log_prob") was not updated during initialization, because the following exception occured: RuntimeError('Error while updating Calc(name="_model_log_prob").'). See debug log for the full traceback.

This is not pretty, because it is just intended behavior and I think probably users should not be warned about this. Before this PR is ready for review, I would like to come up with a nicer solution for this case.

@wiep
Copy link
Contributor

wiep commented Sep 12, 2023

i'm still not super happy with the default warnings. so far, we say liesel you can build a partial graph but that would trigger warnings what is not really what should happen. usually i do not like this magic, but here it might be justified: a context manager that turns the updates on (or off). i think that is better than fiddling with the log levels.

with lsl.instant_evaluation():
     a = lsl.Data(1.0)
     b = lsl.Calc(lambda x: x / 0, a)
# warnings
a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a)
# no warnings

@jobrachem
Copy link
Contributor Author

I don't really think the magic context manager would bring us a lot of joy here. Manually, the desired functionality can already be achieved by

a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a).update()

This makes it more obvious what is happening than the context manager.

The idea of this PR is mostly to make it easier for users to spot errors in their code early without introducing additional manual work. If you don't know the internals of how, when and why exactly a calculator gets updated, it is currently easy to get tripped up. At least that is what happened to me.

I think an alternative to the work this PR could lie in documentation.

i think that is better than fiddling with the log levels.

You only need to change the log levels if you have no other way of accessing the original traceback. When you are working interactively, say in quarto, you can just run b.update() manually one time to get the error message. If you can't do that, activating the "debug" log-level for debugging is reasonable I think.

What is true is that this code looks a little fiddly:

logger = logging.getLogger("liesel")
logger.handlers[0].setLevel(logging.DEBUG)

This is a result of how we set up our logging. I think we could (and should) change the setup such that you can set the log level like this:

logger = logging.getLogger("liesel")
logger.setLevel(logging.DEBUG)

But that is a different issue.

@wiep
Copy link
Contributor

wiep commented Sep 17, 2023

Building a non-initialized model is possible without warnings is imho a crucial aspect of Liesel. However, the proposed implementation may require users to turn off logging, build the non-initialized model, and then turn the logging back on. I find that not ideal. Checking the calculations during initialization is often beneficial from a user's perspective, though. Therefore, I think, it may be necessary to have two different modes to cater to both scenarios.

One possible solution is to enable updates by default and disable them only when a particular setting is altered. For example, a ctx manager could be used to adjust the settings temporarily.

lsl.update_on_construction = False
# build model

# or

with lsl.disable_updates_on_construction():
    # build model

We can discuss this further during one of our upcoming weekly meetings.

jobrachem and others added 13 commits September 27, 2023 15:22
Co-authored-by: Gianmarco Callegher <[email protected]>
* Efficient MVN Degenerate

* Fixed blackjax refactor error

* Refactor

* indent more code

* Update CHANGELOG.md

---------

Co-authored-by: Gianmarco Callegher <[email protected]>
Co-authored-by: Johannes Brachem <[email protected]>
* Update summary_m.py

* Update CHANGELOG.md
@jobrachem
Copy link
Contributor Author

We could also simply add an init parameter to calculators:

a = lsl.Data(1.0)
b = lsl.Calc(lambda x: x / 0, a, update_on_init=False)

This could be set to True by default.

@jobrachem
Copy link
Contributor Author

We will go with the init argument, which defaults to True. We can add a context manager later, if it turns out we want one.

GianmarcoCallegher and others added 5 commits January 31, 2024 15:18
…#167)

* Transform method handles nodes duplicates

* Refactor

* Added tests. Removed useless check

* Update CHANGELOG.md

* rename and move test

* rephrase message

* remove unnecessary code

---------

Co-authored-by: Gianmarco Callegher <[email protected]>
Co-authored-by: Johannes Brachem <[email protected]>
Due to the exception being caught and re-raised in the update() method, we do not get the actual error text here.
@jobrachem jobrachem marked this pull request as ready for review January 31, 2024 14:38
@jobrachem
Copy link
Contributor Author

@wiep if you got a notification for this PR, you can ignore it. @GianmarcoCallegher is doing the review.

@GianmarcoCallegher
Copy link
Contributor

LGTM

@GianmarcoCallegher GianmarcoCallegher merged commit df53f7d into main Feb 9, 2024
3 checks passed
@GianmarcoCallegher GianmarcoCallegher deleted the update-calc-on-init branch February 9, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call Calc.update() once as the last step of initialization
5 participants