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 misc. issues surrounding ProfileStats #1121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Aug 17, 2022

This PR fixes some of the issues mentioned in #1120 and provides some general refactoring for ProfileStats and related code.

More specifically, this PR removes the atexit functionality that was already very buggy and completely untested.

@brandonwillard brandonwillard added bug Something isn't working refactor This issue involves refactoring labels Aug 17, 2022
@brandonwillard brandonwillard linked an issue Aug 17, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #1121 (926cd2e) into main (9f176da) will increase coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
+ Coverage   79.25%   79.30%   +0.05%     
==========================================
  Files         159      159              
  Lines       48097    48051      -46     
  Branches    10934    10920      -14     
==========================================
- Hits        38117    38105      -12     
+ Misses       7469     7434      -35     
- Partials     2511     2512       +1     
Impacted Files Coverage Δ
aesara/compile/profiling.py 77.56% <31.25%> (+3.01%) ⬆️
aesara/compile/function/types.py 79.03% <33.33%> (+0.02%) ⬆️
aesara/compile/function/pfunc.py 83.41% <100.00%> (ø)
aesara/scan/utils.py 87.50% <100.00%> (ø)

@mattearllongshot
Copy link

mattearllongshot commented Aug 19, 2022

Hello, I just tested this, but I don't see any output on stderr:

$ cat profile_simple.py
import aesara
import aesara.tensor as at
import numpy as np


params = at.vector()

f = aesara.function([params], [params ** 2])

print(f(np.array([1, 2, 3])))
$ AESARA_FLAGS=profile=True python profile_simple.py
[array([1., 4., 9.])]
$

@brandonwillard
Copy link
Member Author

Hello, I just tested this, but I don't see any output on stderr:

Yes, the atexit stuff has been removed in this PR. You will need to print or save the profile information in your script.

That atexit code was a mess, and the whole approach has its own issues that don't appear to be worth all the extra effort.

I haven't merged this PR because I still haven't decided what to do about this situation, so, if you have some use-cases specific to the atexit functionality, tell us and we might be able to justify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor This issue involves refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests and fixes needed for aesara.compile.profiling
2 participants