Remove unused config options #806

Armavica · 2024-06-06T00:27:55Z

Description

Remove unused config options
Add type hints
Fix broken check_duplicate_key.py
Replace os.path with pathlib
- Most of the remaining uses are in cmodule.py which I didn't do because it's a lot of work and I am understanding that it is going to go soon.

Related Issue

Closes #
Related to #

Checklist

Checked that the pre-commit linting/style checks pass
Included tests that prove the fix is effective or that the new feature works
Added necessary documentation (docstrings and/or example notebooks)
If you are a pro: each commit corresponds to a relevant logical change

Type of change

pytensor/configparser.py

codecov · 2024-06-18T18:48:58Z

Codecov Report

Attention: Patch coverage is 68.58238% with 82 lines in your changes missing coverage. Please review.

Project coverage is 81.34%. Comparing base (10f285a) to head (8830004).
Report is 104 commits behind head on main.

Files with missing lines	Patch %	Lines
pytensor/compile/compiledir.py	0.00%	16 Missing ⚠️
pytensor/configdefaults.py	44.82%	13 Missing and 3 partials ⚠️
pytensor/d3viz/d3viz.py	25.00%	9 Missing ⚠️
pytensor/link/c/lazylinker_c.py	55.00%	4 Missing and 5 partials ⚠️
pytensor/misc/check_duplicate_key.py	0.00%	9 Missing ⚠️
pytensor/printing.py	11.11%	8 Missing ⚠️
pytensor/link/c/cutils.py	53.84%	3 Missing and 3 partials ⚠️
pytensor/tensor/blas_headers.py	75.00%	3 Missing ⚠️
pytensor/compile/profiling.py	50.00%	1 Missing ⚠️
pytensor/configparser.py	98.98%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #806      +/-   ##
==========================================
+ Coverage   81.23%   81.34%   +0.10%     
==========================================
  Files         170      170              
  Lines       46826    46833       +7     
  Branches    11465    11432      -33     
==========================================
+ Hits        38041    38097      +56     
+ Misses       6594     6549      -45     
+ Partials     2191     2187       -4

Files with missing lines	Coverage Δ
pytensor/compile/compilelock.py	`97.14% <100.00%> (+0.08%)`	⬆️
pytensor/link/c/cmodule.py	`56.88% <100.00%> (ø)`
pytensor/scalar/math.py	`87.08% <100.00%> (-0.18%)`	⬇️
pytensor/compile/profiling.py	`76.08% <50.00%> (+0.03%)`	⬆️
pytensor/configparser.py	`89.84% <98.98%> (+3.48%)`	⬆️
pytensor/d3viz/formatting.py	`12.88% <50.00%> (ø)`
pytensor/link/c/op.py	`62.17% <93.33%> (-0.09%)`	⬇️
pytensor/misc/elemwise_openmp_speedup.py	`25.58% <50.00%> (+1.77%)`	⬆️
pytensor/tensor/io.py	`77.41% <66.66%> (+0.75%)`	⬆️
pytensor/tensor/blas_headers.py	`45.55% <75.00%> (-2.84%)`	⬇️
... and 7 more

pytensor/compile/compiledir.py

pytensor/configdefaults.py

pytensor/link/c/cmodule.py

pytensor/link/c/op.py

lucianopaz

Looks like an awful lot of long, detailed and tedious work. Great job @Armavica! I left a few comments but once those are done, I think this should be good to go.

pytensor/compile/compiledir.py

pytensor/compile/compilelock.py

lucianopaz · 2024-06-21T09:28:21Z

pytensor/configdefaults.py

-    path = os.path.realpath(path)
-    if os.access(path, os.F_OK):  # Do it exist?
+    path = path.resolve()
+    if path.exists():
        if not os.access(path, os.R_OK | os.W_OK | os.X_OK):


This could be changed as well to use path.stat().st_mode, to avoid using os.access, but it might not be worth it.

Yes indeed, but not sure it's worth it indeed. I think it would make this code more verbose.

Armavica · 2024-07-06T19:36:11Z

@ricardoV94 I am not sure how to handle this PR regarding labels: Should the removal of unused config options be considered major?

ricardoV94 · 2024-07-06T20:02:50Z

@ricardoV94 I am not sure how to handle this PR regarding labels: Should the removal of unused config options be considered major?

Nah, because I expect exactly 0 people were trying to set it

ricardoV94 · 2024-07-06T20:04:07Z

Do you want to squash or rebase and merge?

Armavica · 2024-07-06T20:12:13Z

Do you want to squash or rebase and merge?

I would rather not squash, having all the commits makes it easier to bisect and debug if needed. But if you think this is too many commits I can rebase and make less of them.

ricardoV94 · 2024-07-06T20:54:43Z

Yeah maybe the pathlib ones can be merged together

pytensor/scalar/math.py

And add type hints to configparser.py

maresb · 2024-07-09T17:50:41Z

Ugh, Windows strikes again. @Armavica, @ricardoV94, please see https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=974087&view=logs&jobId=7df5a329-fcdb-5857-5720-a5dae94c0641&j=7df5a329-fcdb-5857-5720-a5dae94c0641&t=67fc0302-7ecf-51cb-2fd0-391a1258793e

Not sure exactly what's wrong or how to fix it. 😞

Armavica · 2024-07-09T18:40:49Z

Ah, a problem that could have been caught with more types!

ricardoV94 · 2024-07-09T19:18:24Z

Ah, a problem that could have been caught with more types!

And tests :D

Armavica force-pushed the pathlib branch 3 times, most recently from 244d74e to d92cef9 Compare June 6, 2024 01:49

Armavica changed the title ~~Use pathlib rather than os.path~~ Use pathlib rather than os.path and start typing ConfigParser Jun 6, 2024

Armavica force-pushed the pathlib branch from d92cef9 to 2f1de24 Compare June 6, 2024 09:55

ricardoV94 reviewed Jun 6, 2024

View reviewed changes

pytensor/configparser.py Outdated Show resolved Hide resolved

Armavica force-pushed the pathlib branch from 2f1de24 to 550dbd0 Compare June 7, 2024 11:51

ricardoV94 reviewed Jun 7, 2024

View reviewed changes

pytensor/configparser.py Outdated Show resolved Hide resolved

Armavica force-pushed the pathlib branch 2 times, most recently from d94df18 to c0cf269 Compare June 7, 2024 12:14

Armavica force-pushed the pathlib branch 3 times, most recently from 36c93d4 to da6383c Compare June 18, 2024 18:26

Armavica force-pushed the pathlib branch from da6383c to e30f814 Compare June 18, 2024 18:53

Armavica marked this pull request as ready for review June 18, 2024 19:14

Armavica requested a review from ricardoV94 June 18, 2024 19:14

ricardoV94 requested review from michaelosthege and lucianopaz June 19, 2024 13:26

michaelosthege reviewed Jun 20, 2024

View reviewed changes

Armavica force-pushed the pathlib branch 2 times, most recently from db1f5d2 to b607e55 Compare June 20, 2024 21:49

lucianopaz reviewed Jun 21, 2024

View reviewed changes

Armavica force-pushed the pathlib branch 2 times, most recently from b189749 to 90ecf06 Compare June 21, 2024 15:13

Armavica force-pushed the pathlib branch from 90ecf06 to 1575bb0 Compare July 1, 2024 09:13

Armavica requested a review from lucianopaz July 1, 2024 11:16

michaelosthege approved these changes Jul 1, 2024

View reviewed changes

Armavica force-pushed the pathlib branch from 1575bb0 to 4529822 Compare July 3, 2024 09:26

Armavica changed the title ~~Use pathlib rather than os.path and start typing ConfigParser~~ Typing ConfigParser, removing unused config options and using pathlib instead of os.path Jul 6, 2024

Armavica changed the title ~~Typing ConfigParser, removing unused config options and using pathlib instead of os.path~~ Remove unused config options, add type hints and replace os.path with pathlib Jul 6, 2024

Armavica force-pushed the pathlib branch from 4529822 to 67172e0 Compare July 6, 2024 21:11

Armavica added the do not merge label Jul 6, 2024

Armavica force-pushed the pathlib branch from 67172e0 to ef87226 Compare July 6, 2024 21:16

michaelosthege reviewed Jul 6, 2024

View reviewed changes

pytensor/scalar/math.py Outdated Show resolved Hide resolved

Armavica force-pushed the pathlib branch from ef87226 to 3a144c9 Compare July 7, 2024 00:17

Armavica added the bug Something isn't working label Jul 7, 2024

Armavica force-pushed the pathlib branch from 3a144c9 to 4dd3d8e Compare July 7, 2024 06:10

Armavica removed the do not merge label Jul 7, 2024

Armavica force-pushed the pathlib branch 4 times, most recently from e3faa72 to d2106bf Compare July 7, 2024 20:04

Armavica added 3 commits July 9, 2024 10:35

Replace os.path with pathlib

1856627

And add type hints to configparser.py

Remove unused config options

24af8ec

Fix check_duplicate_key.py and remove os.path

8830004

Armavica force-pushed the pathlib branch from d2106bf to 8830004 Compare July 9, 2024 14:36

Armavica changed the title ~~Remove unused config options, add type hints and replace os.path with pathlib~~ Remove unused config options Jul 9, 2024

Armavica merged commit 2effaf1 into main Jul 9, 2024
58 of 59 checks passed

Armavica deleted the pathlib branch July 9, 2024 15:01

Armavica mentioned this pull request Jul 9, 2024

Fix Windows build #909

Merged

11 tasks

Armavica mentioned this pull request Jul 13, 2024

Fix supplying (base_)compiledir as str #927

Merged

11 tasks

Remove unused config options #806

Remove unused config options #806

Uh oh!

Conversation

Armavica commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Checklist

Type of change

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucianopaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucianopaz Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

Armavica Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

Armavica commented Jul 6, 2024

Uh oh!

ricardoV94 commented Jul 6, 2024

Uh oh!

ricardoV94 commented Jul 6, 2024

Uh oh!

Armavica commented Jul 6, 2024

Uh oh!

ricardoV94 commented Jul 6, 2024

Uh oh!

Uh oh!

Uh oh!

maresb commented Jul 9, 2024

Uh oh!

Armavica commented Jul 9, 2024

Uh oh!

ricardoV94 commented Jul 9, 2024

Uh oh!

Uh oh!

Armavica commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading