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

Move aesara-cache into aesara.bin package #1382

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jan 2, 2023

As observed in #1375 (comment),

Additionally, I noticed that aesara-cache is being installed in a top-level module called bin. This seems very undesirable, as it should probably instead go under aesara.bin. I will make a subsequent PR to address this.

In order to hopefully avoid breaking existing stuff, the old scripts are still there, but now with deprecation warnings.

Thank you for opening a PR!

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

@maresb maresb marked this pull request as ready for review January 2, 2023 23:12
@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #1382 (4b9b10d) into main (0f8d849) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

❗ Current head 4b9b10d differs from pull request most recent head 4777151. Consider uploading reports for the commit 4777151 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1382      +/-   ##
==========================================
- Coverage   74.69%   74.59%   -0.10%     
==========================================
  Files         195      196       +1     
  Lines       49748    49813      +65     
  Branches    10529    10547      +18     
==========================================
  Hits        37157    37157              
- Misses      10266    10331      +65     
  Partials     2325     2325              
Impacted Files Coverage Δ
aesara/bin/aesara_cache.py 0.00% <0.00%> (ø)

@maresb
Copy link
Contributor Author

maresb commented Jan 3, 2023

Maybe there should be test coverage for aesara-cache?

@brandonwillard brandonwillard added enhancement New feature or request refactor This issue involves refactoring labels Jan 3, 2023
@brandonwillard
Copy link
Member

Maybe there should be test coverage for aesara-cache?

Yeah, we (and Theano) badly need that.

@maresb
Copy link
Contributor Author

maresb commented Jan 3, 2023

and Theano

I thought Aesara officially took over Theano.

@brandonwillard
Copy link
Member

and Theano

I thought Aesara officially took over Theano.

Yes; I'm just saying that I don't recall inheriting such tests from Theano.

@brandonwillard brandonwillard force-pushed the aesara-cache-into-aesara-package branch from 1cd40bd to bb969ee Compare January 7, 2023 00:35
@brandonwillard brandonwillard enabled auto-merge (rebase) January 7, 2023 00:36
brandonwillard
brandonwillard previously approved these changes Jan 7, 2023
@brandonwillard brandonwillard force-pushed the aesara-cache-into-aesara-package branch from 4b9b10d to 4777151 Compare January 9, 2023 05:32
@maresb maresb merged commit 96eca11 into aesara-devs:main Jan 9, 2023
@maresb maresb deleted the aesara-cache-into-aesara-package branch January 9, 2023 08:35
@maresb
Copy link
Contributor Author

maresb commented Jan 9, 2023

Oh, nice! I'm glad it merged.

Do you understand what went wrong earlier with the coverage check? (It was failing on aesara_cache.py even after I had added it to the omit list.)

@maresb
Copy link
Contributor Author

maresb commented Jan 9, 2023

Oh, probably I should have added it to [tool.coverage.run] instead of [tool.coverage.report]...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor This issue involves refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants