Use time.perf_counter for aesara.function timing results

[anirudhacharya]

closes #1246

Changing the timing function for high-precision performance measurement.

Note to reviewers - time_ns() function is only available from Python 3.7 onwards. Let me know if that is an issue for the library.

[anirudhacharya]

Hello reviewers, this is my first PR in this repo, I will try to make more PRs in the future.

I wanted to know more about this project and about Theano's/Aesara's symbolic computation graphs, so I thought making a few PRs on the way might be good.

A couple of questions -

1. I tried running pytest tests/compile/function/test_types.py locally but I got the following error -

ImportError while importing test module '/Users/anirudhacharya/Code/aesara/tests/compile/function/test_types.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../.pyenv/versions/3.7.5/lib/python3.7/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/compile/function/test_types.py:7: in <module>
    import aesara.tensor as at
aesara/__init__.py:64: in <module>
    from aesara.configdefaults import config
aesara/configdefaults.py:11: in <module>
    from setuptools._distutils.spawn import find_executable
E   ModuleNotFoundError: No module named 'setuptools._distutils'

but I also got the same error with the latest commit on the main branch, so I don't think the error has anything to do with the changes in this PR. Is there some setup or installation that I am missing? I am able to successfully use pytest on other python projects on my machine.

2. I would also like to add this Add aesara.as_symbolic to the documentation #746 in this PR itself. Can someone point me where in the documentation the as_symbolic function might go, would it be here - https://github.com/aesara-devs/aesara/blob/main/doc/library/graph/graph.rst

@brandonwillard @michaelosthege @ricardoV94 @rlouf

[rlouf]

but I also got the same error with the latest commit on the main branch, so I don't think the error has anything to do with the changes in this PR. Is there some setup or installation that I am missing? I am able to successfully use pytest on other python projects on my machine.

I reran the test on the main branch to check, but they run without problem. Did you create a fresh environment and follow the installation instruction for developers ? Let's see if the tests pass in CI.

2. I would also like to add this Add aesara.as_symbolic to the documentation #746 in this PR itself. Can someone point me where in the documentation the as_symbolic function might go, would it be here - https://github.com/aesara-devs/aesara/blob/main/doc/library/graph/graph.rst

It is better to open separate PRs for separate issues unless they're directly related. It makes the review process easier and generally means we can merge PRs faster. It is also best to centralize the information and ask questions in the issue directly. I added a suggestion there.

[codecov]

Codecov Report

Merging #1262 (fd8d2da) into main (471657a) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1262   +/-   ##
=======================================
  Coverage   74.10%   74.10%           
=======================================
  Files         174      174           
  Lines       48624    48624           
  Branches    10351    10351           
=======================================
  Hits        36035    36035           
  Misses      10301    10301           
  Partials     2288     2288           

Impacted Files	Coverage Δ
aesara/compile/function/types.py	79.00% <100.00%> (ø)

[rlouf]

Please capitalize your commit as explained in this reference in the PR checklist and use a message more explicit than "refactor X", such as e.g. "Use time.time_ns() instead of time.time()". LGTM otherwise.

[anirudhacharya]

Please capitalize your commit as explained in this reference in the PR checklist and use a message more explicit than "refactor X", such as e.g. "Use time.time_ns() instead of time.time()". LGTM otherwise.

I have updated the commit message as requested.

[anirudhacharya]

I reran the test on the main branch to check, but they run without problem. Did you create a fresh environment and follow the installation instruction for developers ? Let's see if the tests pass in CI.

The CI did pass( the first run) with all green, so I did a git clean and followed the instructions on the link you gave to rebuild and rerun the tests. I did not get the error that I mentioned before, but a few of the tests did fail with the error CompilationFailed, and I am trying to resolve them here in this issue #1263

[anirudhacharya]

The latest test failures on the CI are due to the following -

ERROR: Could not install packages due to an OSError: HTTPSConnectionPool(host='files.pythonhosted.org', port=443): Max retries exceeded with url: /packages/94/b3/ff2845971788613e646e667043fdb5f128e2e540aefa09a3c55be8290d6d/filelock-3.8.0-py3-none-any.whl (Caused by NewConnectionError('<pip._vendor.urllib3.connection.HTTPSConnection object at 0x7f8f4072ae20>: Failed to establish a new connection: [Errno 101] Network is unreachable'))

It was some sort of a network issue, I think it might pass if we reran the tests.

[brandonwillard]

Thanks for the PR!

Before going any further, we need to address #1246 (comment).

[brandonwillard]

I'm not sure what's up, but I'm seeing Codecov warnings in the full "Files changed" diff for code that wasn't changed. Perhaps we need to update the Codecov settings?

[anirudhacharya]

I'm not sure what's up, but I'm seeing Codecov warnings in the full "Files changed" diff for code that wasn't changed. Perhaps we need to update the Codecov settings?

@brandonwillard I am not sure how the CI is setup, but codecov settings seem to be coming from here - https://github.com/aesara-devs/aesara/blob/be0ea5ca1c13362be28fc209789cc59816893f17/codecov.yml#L18y

Instead of maybe changing/loosening the settings, can we check why codecov raised the error -
Added line #L47 was not covered by tests
when that line was not added in this PR at all?

It would seem my PR is not the only one that is facing this issue, and this issue might be flaky( codecov/patch failed but codecov/project passed) - https://github.com/aesara-devs/aesara/pull/1245/checks?check_run_id=8918034749.

Can we just rerun only codecov for this PR and see if the error repeats?

[brandonwillard]

@brandonwillard I am not sure how the CI is setup, but codecov settings seem to be coming from here - https://github.com/aesara-devs/aesara/blob/be0ea5ca1c13362be28fc209789cc59816893f17/codecov.yml#L18y

Well, it looks like the issue has gone away by itself, because now I don't see all those beta-feature annotations any more. We could likely make some improvements to the Codecov settings, though.

Regardless, it looks like a merge commit has been added, so we'll need to rebase this and remove it in order to merge.

[brandonwillard]

These changes seem fine. The only questions I have are:

• Should we use time.perf_counter_ns instead?
• Do we want to make the timer/counter configurable?

The first question is the only one that's relevant to merging this PR right now, and it probably isn't very important. The second question could/should easily be its own issue and/or PR.

[anirudhacharya]

Regardless, it looks like a merge commit has been added, so we'll need to rebase this and remove it in order to merge.

If we do a squash and merge this will not be a problem.

• Should we use time.perf_counter_ns instead?

This sounds reasonable. Also, doesn't it make sense to record it as ns itself, instead of dividing by 10**9 before recording the time?

• Do we want to make the timer/counter configurable?

Configurable, how? What needs to be configured?

[brandonwillard]

If we do a squash and merge this will not be a problem.

Whichever way is fine; I just want to be clear that this branch can't be added to main in its current form.

This sounds reasonable. Also, doesn't it make sense to record it as ns itself, instead of dividing by 10**9 before recording the time?

Yes, that's one of the things that made me think about it.

Configurable, how? What needs to be configured?

For instance, if one wanted to use time.time_ns instead, we could provide and option for that. At best, it might be worth creating an issue for the idea, but it would be very low priority.

[brandonwillard]

To keep things simple, I'm going to merge this now and we can follow up with new issues for the other ideas mentioned here, if need be.