[BUG] fix AttributeError: 'ExperimentWriter' object has no attribute 'add_figure'

[ewth]

Description

This PR fixes #1256.

At present, the AttributeError is raised (and the script exits) if the logger used does not have the add_figure method.

This fix adds a simple check prior to calling add_figure.

Checklist

• Linked issues (if existing)
• Amended changelog for large changes (and added myself there as contributor)
• Added/modified tests
• Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with pre-commit install.
  To run hooks independent of commit, execute pre-commit run --all-files

Make sure to have fun coding!

[ewth]

Note that I've added an additional check for add_embedding, which causes a similar AttributeError if the logger doesn't have the add_embedding method.

Although related, it's not specifically about the linked issue. If preferred, I can create a new issue and split that off into it.

[ewth]

I have not run pre-commit on these commits -- I was having trouble running the toolchain at all. To "test" the fix I just instantiated the model from the modified Python and ran training on it. Happy to follow any advice/suggestions on this.

$ pre-commit run --all-files
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: AssertionError: BUG: expected environment for python to be healthy() immediately after install, please open an issue describing your environment
Check the log at /.../.cache/pre-commit/pre-commit.log

pre-commit.log:

version information

pre-commit version: 2.17.0
git --version: git version 2.34.1
sys.version:
    3.10.12 (main, Sep 11 2024, 15:47:36) [GCC 11.4.0]
sys.executable: /usr/bin/python3
os.name: posix
sys.platform: linux

error information

An unexpected error has occurred: AssertionError: BUG: expected environment for python to be healthy() immediately after install, please open an issue describing your environment

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/pre_commit/error_handler.py", line 70, in error_handler
    yield
  File "/usr/lib/python3/dist-packages/pre_commit/main.py", line 396, in main
    return run(args.config, store, args)
  File "/usr/lib/python3/dist-packages/pre_commit/commands/run.py", line 416, in run
    install_hook_envs(to_install, store)
  File "/usr/lib/python3/dist-packages/pre_commit/repository.py", line 224, in install_hook_envs
    _hook_install(hook)
  File "/usr/lib/python3/dist-packages/pre_commit/repository.py", line 86, in _hook_install
    raise AssertionError(
AssertionError: BUG: expected environment for python to be healthy() immediately after install, please open an issue describing your environment

[ewth]

Ah!

I just realised pre-commit was installed at a system level, not in the environment I was using. I can now run it successfully.

Running it on this branch produces the following:

$ pre-commit run --all-files
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

examples/ar.py:88:56: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {deepar.size()/1e3:.1f}k")
                                                       ^
examples/nbeats.py:68:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {net.size()/1e3:.1f}k")
                                                    ^
examples/stallion.py:122:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {tft.size()/1e3:.1f}k")
                                                    ^
pytorch_forecasting/data/timeseries.py:106:28: E226 missing whitespace around arithmetic operator
                f"{na} ({na/tensor.size(0):.2%}) of {name} "
                           ^

isort....................................................................Passed
black....................................................................Passed
nbqa-black...............................................................Passed
nbqa-isort...............................................................Passed
nbqa-flake8..............................................................Failed
- hook id: nbqa-flake8
- exit code: 1

docs/source/tutorials/stallion.ipynb:cell_7:25:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {tft.size()/1e3:.1f}k")
                                                    ^
docs/source/tutorials/stallion.ipynb:cell_9:29:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {tft.size()/1e3:.1f}k")
                                                    ^

nbqa-check-ast...........................................................Passed

Which is the same result as running it on main:

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

examples/ar.py:88:56: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {deepar.size()/1e3:.1f}k")
                                                       ^
examples/nbeats.py:68:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {net.size()/1e3:.1f}k")
                                                    ^
examples/stallion.py:122:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {tft.size()/1e3:.1f}k")
                                                    ^
pytorch_forecasting/data/timeseries.py:106:28: E226 missing whitespace around arithmetic operator
                f"{na} ({na/tensor.size(0):.2%}) of {name} "
                           ^

isort....................................................................Passed
black....................................................................Passed
nbqa-black...............................................................Passed
nbqa-isort...............................................................Passed
nbqa-flake8..............................................................Failed
- hook id: nbqa-flake8
- exit code: 1

docs/source/tutorials/stallion.ipynb:cell_7:25:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {tft.size()/1e3:.1f}k")
                                                    ^
docs/source/tutorials/stallion.ipynb:cell_9:29:53: E226 missing whitespace around arithmetic operator
print(f"Number of parameters in network: {tft.size()/1e3:.1f}k")
                                                    ^

nbqa-check-ast...........................................................Passed

[ewth]

I've created #1695 and #1696 as a result of the above.

[fkiraly]

Hm, question - is this an issue if lightning >=2, or only on lightning <2?

Because currently, pyproject.toml does not officially support lightning <2.

Nothing wrong with more defensive programming, but it feels like there is a larger issue to discuss about lightning <2 support if this is the case.

[ewth]

It impacts me with lightning==2.4.0.

[fkiraly]

But that would mean the functionality is no longer available on higher lightning versions?

We need to understand then the version range this issue appears on, the exact condition when it occurs, and why we are currently not seeing it in the tests: the version in the CI is lightning 2.4.0.

I believe we are covering and executing the code in question?

[fkiraly]

@ewth, ping - could you kindly explain the exact condition under which the issue arises that you are fixing? I cannot reproduce the error, and there does not seem to be a bug report.

Update: I saw the bug report, I missed your posts there. It is lightning without tensorboard, as this affects the logger on newer versions, correct?

[fkiraly]

Looks good to me.

May I request some small things:

• the hasattr check assumes logger has an attr experiment. Is this even always true? Given the unclear interface changes to the loggers, I would just be more defensive, and check that logger has experiment and that has add_figure.
• there is now a lot of duplicated code, not very DRY. May I suggest to move this to the base model into a function _logger_supports(fun : str)? The fun is a string that can be "add_figure" or "add_experiment". For "add_figure", we would check both that matplotlib is present, and carry out the new check.

[ewth]

@fkiraly I’ve been buried in work — I haven’t abandoned this, and will return to it as soon as I am able (hopefully this week sometime).

[fkiraly]

sure, no rush. Worst case, it is very close to the finish line and a maintainer will wrap it up sometime.

[fkiraly]

@ewth, are you still planning to wrap this up? We are approaching a new release.

[fkiraly]

I made the requested changes now so this can go into the release. Thanks for your work, @ewth!

[ewth]

Thanks @fkiraly! Sorry I haven't been able to get back to it, 110% of my time has been consumed of late.

Keen to help out again in the (hopefully near) future when I can. I've found pytorch-forecasting to be a very useful tool so definitely want to try and give back to the project.