BUG: Fix miscellaneous errors in scripts #77

jhlegarreta · 2024-12-30T17:16:04Z

BUG: Add the py extension to the script filenames
BUG: Fix Mapping and Iterables names import modules
BUG: Rename scripts: do not use package names to name scripts

jhlegarreta · 2024-12-30T17:20:17Z

CI failure is unrelated:
https://github.com/demianw/tract_querier/actions/runs/12549690221/job/34991201789?pr=77#step:6:136

X-ref issue #69.

jhlegarreta · 2024-12-30T21:19:33Z

I know 8b32231 is probably not a popular decision, but I see no other way to solve the script and package name clash issue. Not sure why this did not happen before.

Also, I do not know whether this implies changes to the wmql.py module, especially to:

tract_querier/tract_querier/nipype/wmql.py

Line 40 in b8c54c5

    
               'tract_querier -q freesurfer_queries.qry -a wmparc.nii.gz -t tracts.vtk -o query_ --query_selection ilf.left,ilf.right'

tract_querier/tract_querier/nipype/wmql.py

Line 44 in b8c54c5

_cmd = 'tract_querier'

jhlegarreta · 2025-01-04T14:50:34Z

If the current renaming is not good/descriptive enough for the package, other possibilities may be: tract_querier_query_tracts.py, tract_querier_apply_tract_maths.py or the more compact tq_query_tracts.py, tq_apply_tract_maths.py.

demianw · 2025-01-13T15:24:17Z

I'm really not keen on renaming the tools. Can we avoid it? there was never an issue before. I can check if we can avoid renaming it. But having a script with the same name of the package is common practice

jhlegarreta · 2025-01-13T15:40:03Z

Can we avoid it?

I have not found a way to rename them, unfortunately. TBH, I believe that keeping naming a script with the same name as the package is something that will continue causing issues and prevent using the tool for those of us that likely need to develop on top of it (i.e. not only call the scripts). The alternative is to rename the package, which involves more work and looks less appropriate as a fix here. From the point of view of a SW, a script name should take the form of an action verb. As long as all this is clear in the documentation, I do not think this will be misleading, and I do not think users of tract_querier will stop using the tool if the scripts are renamed. Adding the path of the scripts to the PATH is likely not to be an ideal solution and may not work for those that are using the tool to develop on top of it.

If you find a solution that avoids renaming the scripts that is not likely to cause issues such as the ones described above, I would be happy to try them.

I will rebase on master, fix conflicts and force push later.

demianw · 2025-01-13T15:47:21Z

I don't share the opinion that this will cause problems. There are several examples on packages with the same name as their scripts, for example: https://sshuttle.readthedocs.io/en/stable/

jhlegarreta · 2025-01-13T22:37:24Z

Sorry, Demian, re-checked my statements and went through the example you provided me with:

The sshuttle package does not have any file that is named sshuttle; the sshuttle script comes from
https://github.com/sshuttle/sshuttle/blob/master/setup.py#L48
https://github.com/sshuttle/sshuttle/blob/master/sshuttle/cmdline.py

i.e. the Python will install a script that is named sshuttle in the package binaries, but the content of that script draws from the one in the cmdline.py file.
So, I re-assessed the solution I am proposing here using a fresh build: cloned the repository in another folder, brought the changes in this PR, created a virtual environment, did pip install . and called tract_querier -h and effectively got the help message. That is, this line
https://github.com/demianw/tract_querier/pull/77/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R49

is doing exactly what sshuttle is doing as described above and the following documentation change was wrong:
https://github.com/demianw/tract_querier/pull/77/files#diff-864fe75473c588eca196acdd38666dccc8ac778bf7aa3102eb802110c00b53f5R17

And I am reverting it in the new force push.

The need to rename the script comes from the need to distinguish the script name from the package name to avoid a clash c0ad2a1. But the changes proposed in this PR do not involve changes in the name of the installed script. And hence the I assume the nypipe interface will not be impacted (BUG: Fix miscellaneous errors in scripts #77 (comment)).

Are we OK with this? Please try the above recipe (cloning the repository, checking out the branch, installing the package and calling the help on the script) on your end if you want to be sure.

demianw · 2025-01-14T10:31:22Z

Sounds great, good job! Would it be possible to be more uniform in the renaming of the scripts ? For instance cli_tract_math and cli_tract_querier ?

Rename scripts so that their `main` method can be effectively imported when installing them as Python scripts that can be effectively called from the command line: add the `py` extension to the script filenames. Fixes: ``` Traceback (most recent call last): File ".virtualenvs/tract_querier/bin/tract_querier", line 5, in <module> from scripts.tract_querier import main ModuleNotFoundError: No module named 'scripts.tract_querier' Traceback (most recent call last): File ".virtualenvs/tract_querier/bin/tract_querier", line 5, in <module> from scripts.tract_querier import main ModuleNotFoundError: No module named 'scripts.tract_querier' ``` raised locally when executing: ``` tract_querier -t "${tractogram_fname}" -a "${wmparc_fname}" -q "${query_fname}" -o "${out_fname}" ``` using some local data.

Fix `Mapping` and `Iterables` classes import modules: import them from `collections.abc` instead of from `collections` as they have been moved to the former since Python 3.3. Fixes: ``` Traceback (most recent call last): File ".virtualenvs/tract_querier/bin/tract_math", line 8, in <module> sys.exit(main()) File ".virtualenvs/tract_querier/lib/python3.10/site-packages/scripts/tract_math.py", line 33, in main from tract_querier.tract_math import operations File ".virtualenvs/tract_querier/lib/python3.10/site-packages/tract_querier/tract_math/__init__.py", line 3, in <module> from .decorator import tract_math_operation, TractMathWrongArgumentsError File ".virtualenvs/tract_querier/lib/python3.10/site-packages/tract_querier/tract_math/decorator.py", line 3, in <module> from collections import Mapping, Iterable ImportError: cannot import name 'Mapping' from 'collections' (/usr/lib/python3.10/collections/__init__.py) ``` Documentation: https://docs.python.org/3.3/library/collections.abc.html

Rename scripts: do not use package names to name scripts to avoid clashes. If a script has the same name as the package, importing the package or its submodules within the script may result in Python trying to import the script itself. Fixes: ``` Traceback (most recent call last): File "/snap/pycharm-professional/443/plugins/python-ce/helpers/pydev/pydevd.py", line 1570, in _exec pydev_imports.execfile(file, globals, locals) # execute the script File "/snap/pycharm-professional/443/plugins/python-ce/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile exec(compile(contents+"\n", file, 'exec'), glob, loc) File "/home/jhlegarreta/src/tract_querier/scripts/tract_querier.py", line 286, in <module> main() File "/home/jhlegarreta/src/tract_querier/scripts/tract_querier.py", line 94, in main if os.path.exists(tract_querier.default_queries_folder): AttributeError: module 'tract_querier' has no attribute 'default_queries_folder' ``` raised locally when debugging `tract_querier.py`.

jhlegarreta · 2025-01-14T14:26:25Z

Would it be possible to be more uniform in the renaming of the scripts ? For instance cli_tract_math and cli_tract_querier?

Done in 14a85f8. Force pushed.

demianw · 2025-01-14T14:40:04Z

Good job @jhlegarreta !

demianw · 2025-01-14T15:16:05Z

There's no script name changes, after the installation, the installed scripts that you can run from the commandline have the correct names @tashrifbillah . It's the intermediate scripts to be installed that have been renamed.

tashrifbillah · 2025-01-14T15:18:18Z

Okay, we shall test the current master shortly.

demianw · 2025-01-14T15:42:39Z

Let us know how it goes !

tashrifbillah · 2025-01-14T20:25:05Z

It went fine, thank you.

demianw · 2025-01-15T12:44:50Z

So we can close the corresponding issue and PR you had opened on this matter @tashrifbillah ?

tashrifbillah · 2025-01-15T14:09:34Z

Yes.

jhlegarreta mentioned this pull request Dec 30, 2024

ENH: Test calling the scripts #78

Open

jhlegarreta force-pushed the FixScriptImportErrors branch from 8b32231 to 31715ac Compare December 30, 2024 21:18

jhlegarreta force-pushed the FixScriptImportErrors branch from 31715ac to 3969469 Compare December 30, 2024 21:25

jhlegarreta force-pushed the FixScriptImportErrors branch from 3969469 to c0ad2a1 Compare January 10, 2025 23:17

jhlegarreta force-pushed the FixScriptImportErrors branch from c0ad2a1 to a468e16 Compare January 13, 2025 22:38

jhlegarreta added 3 commits January 14, 2025 09:21

jhlegarreta force-pushed the FixScriptImportErrors branch from a468e16 to 14a85f8 Compare January 14, 2025 14:25

demianw merged commit afb437b into demianw:master Jan 14, 2025
7 checks passed

demianw mentioned this pull request Jan 14, 2025

Installation instruction does not work #84

Closed

jhlegarreta deleted the FixScriptImportErrors branch January 14, 2025 14:51

jhlegarreta mentioned this pull request Jan 16, 2025

Moved the test files to OSF #88

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix miscellaneous errors in scripts #77

BUG: Fix miscellaneous errors in scripts #77

jhlegarreta commented Dec 30, 2024 •

edited

Loading

jhlegarreta commented Dec 30, 2024

jhlegarreta commented Dec 30, 2024 •

edited

Loading

jhlegarreta commented Jan 4, 2025

demianw commented Jan 13, 2025 •

edited

Loading

jhlegarreta commented Jan 13, 2025

demianw commented Jan 13, 2025

jhlegarreta commented Jan 13, 2025 •

edited

Loading

demianw commented Jan 14, 2025

jhlegarreta commented Jan 14, 2025

demianw commented Jan 14, 2025

demianw commented Jan 14, 2025 •

edited

Loading

tashrifbillah commented Jan 14, 2025

demianw commented Jan 14, 2025

tashrifbillah commented Jan 14, 2025

demianw commented Jan 15, 2025

tashrifbillah commented Jan 15, 2025

BUG: Fix miscellaneous errors in scripts #77

BUG: Fix miscellaneous errors in scripts #77

Conversation

jhlegarreta commented Dec 30, 2024 • edited Loading

jhlegarreta commented Dec 30, 2024

jhlegarreta commented Dec 30, 2024 • edited Loading

jhlegarreta commented Jan 4, 2025

demianw commented Jan 13, 2025 • edited Loading

jhlegarreta commented Jan 13, 2025

demianw commented Jan 13, 2025

jhlegarreta commented Jan 13, 2025 • edited Loading

demianw commented Jan 14, 2025

jhlegarreta commented Jan 14, 2025

demianw commented Jan 14, 2025

demianw commented Jan 14, 2025 • edited Loading

tashrifbillah commented Jan 14, 2025

demianw commented Jan 14, 2025

tashrifbillah commented Jan 14, 2025

demianw commented Jan 15, 2025

tashrifbillah commented Jan 15, 2025

jhlegarreta commented Dec 30, 2024 •

edited

Loading

jhlegarreta commented Dec 30, 2024 •

edited

Loading

demianw commented Jan 13, 2025 •

edited

Loading

jhlegarreta commented Jan 13, 2025 •

edited

Loading

demianw commented Jan 14, 2025 •

edited

Loading