🎨 `print` statements → `log` statements #2062

shnizzedy · 2024-02-07T03:15:10Z

Fixes

Related to #2048 by @nx10
Related to #2058 by @shnizzedy

Description

print statements are useful in some situations (e.g., debugging), but should typically be omitted from production code. print statements can lead to the accidental inclusion of sensitive information in logs, and are not configurable by clients, unlike logging statements.

― https://docs.astral.sh/ruff/rules/print/

Like print statements, pprint statements are useful in some situations (e.g., debugging), but should typically be omitted from production code. pprint statements can lead to the accidental inclusion of sensitive information in logs, and are not configurable by clients, unlike logging statements.

― https://docs.astral.sh/ruff/rules/p-print/

Technical details

Because this PR touches so many files, I'm mostly not taking the time to otherwise lint them as part of this change.
C-PAC/.ruff.toml

Line 11 in 71c9e6f

"nibabel" = "nib"

standardizes import nibabel as nib in 71c9e6f (in some places it was import nibabel as nb), but ruff doesn't catch the nipype-style import strings like

C-PAC/CPAC/distortion_correction/distortion_correction.py

Line 500 in 71c9e6f

convert_afni_warp_imports = ["import os", "import nibabel as nb"]

; those are also fixed in this PR
For Function nodes, automatically inserts this line

C-PAC/CPAC/utils/interfaces/function/function.py

Lines 65 to 66 in e460f7d

"from CPAC.utils.monitoring.custom_logging import FMLOGGER, IFLOGGER, UTLOGGER,"

" WFLOGGER"

after the imports so FMLOGGER, IFLOGGER, UTLOGGER, and WFLOGGER are always available in Function nodes as the "nipype.filemanip", "nipype.interface", "nipype.utils", and "nipype.workflow" loggers, respectively

Tests

C-PAC/CPAC/utils/interfaces/tests/test_function.py

Lines 23 to 50 in e460f7d

    
           def faux_fxn(_loggers: bool = True): 
        
               """Require autoassignment (for testing).""" 
        
               if _loggers: 
        
                   return WFLOGGER, IFLOGGER  # noqa: F821 
        
               return luigi_mario  # noqa: F821 
        
           @mark.parametrize("as_module", [True, False]) 
        
           def test_autologger(as_module: bool) -> None: 
        
               """Test autoassignment of global Nipype loggers`.""" 
        
               interface = Function( 
        
                   function=faux_fxn, input_names=["_loggers"], as_module=as_module 
        
               ) 
        
               interface.inputs._loggers = False 
        
               with raises(NameError) as name_error: 
        
                   interface.run() 
        
                   assert "name 'luigi_mario' is not defined" in str(name_error.value) 
        
               interface = Function( 
        
                   function=faux_fxn, 
        
                   input_names=["_loggers"], 
        
                   output_names=["logger", "iflogger"], 
        
                   as_module=as_module, 
        
               ) 
        
               interface.inputs._loggers = True 
        
               res = interface.run() 
        
               assert res.outputs.logger.name == "nipype.workflow" 
        
               assert res.outputs.iflogger.name == "nipype.interface"

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the lint/pep8-boolean-comparison branch of the repository.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made.
I updated the changelog.
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

fix: python 2 Function interfaces recompatibility

Revert "fix: python 2 Function interfaces recompatibility"

Sty/pepping

STY: Black

(some were `nibabel as nb`)

Where reformatting changed things like > ("line 1" > " line 2") to like > "line 1" " line 2" , change to like > "line 1 line 2"

…pype 1.8.6

shnizzedy

Where I came upon the pattern:

print(error_message)
sys.exit(exitcode)

, I wasn't consistent. Mostly I updated to something like:

logger.error(error_message)
sys.exit(exitcode)

but sometimes I did like:

raise Exception(error_message)

(but with a more specific error class than Exception). I think we generally want to convert all the sys.exits to raise Exceptions, but this PR's already uncomfortably large without doing so. I think that can be a subsequent PR if we care to.

shnizzedy · 2024-02-17T02:40:47Z

CPAC/isc/isfc.py

This file and CPAC/isc/isc.py are almost identical. Not for this PR, but we might want to DRY these files up.

shnizzedy · 2024-02-17T02:43:46Z

CPAC/pipeline/cpac_group_runner.py

@@ -175,6 +178,10 @@ def gather_nifti_globs(pipeline_output_folder, resource_list, pull_func=False):
    # remove any extra /'s


Not for this PR, but

C-PAC/CPAC/pipeline/cpac_group_runner.py

Lines 178 to 191 in 25ea215

# remove any extra /'s

pipeline_output_folder = pipeline_output_folder.rstrip("/")

logger.info(

"\n\nGathering the output file paths from %s...", pipeline_output_folder

)

# this is just to keep the fsl feat config file derivative_list entries

# nice and lean

dirs_to_grab = []

for derivative_name in derivative_list:

for resource_name in resource_list:

if resource_name in derivative_name:

dirs_to_grab.append(derivative_name)

and

C-PAC/CPAC/pipeline/cpac_group_runner.py

Lines 394 to 407 in 25ea215

# remove any extra /'s

pipeline_output_folder = pipeline_output_folder.rstrip("/")

logger.info(

"\n\nGathering the output file paths from %s...", pipeline_output_folder

)

# this is just to keep the fsl feat config file derivatives entries

# nice and lean

search_dirs = []

for derivative_name in derivatives:

for resource_name in resource_list:

if resource_name in derivative_name:

search_dirs.append(derivative_name)

could be DRY'd into a single function

shnizzedy · 2024-02-17T02:59:07Z

CPAC/utils/extract_data_multiscan.py

@@ -276,6 +301,7 @@ def print_scan_param(index):
                                " parameters csv file" % (subject_map.get(sub), scan[0])
                            )

+                    logger.info("site for sub %s -> %s", sub, subject_map.get(sub))


This obviously hasn't been a problem functionally since it's been this way for a long time, but only the first of these many print statements goes to stdout; the rest go to a file.

C-PAC/CPAC/utils/extract_data_multiscan.py

Lines 256 to 267 in dd45308

print("site for sub", sub, "->", subject_map.get(sub))

print(" scan_parameters: ", file=f)

print(" tr:", file=f)

print_scan_param(4)

print(" acquisition:", file=f)

print_scan_param(0)

print(" reference:", file=f)

print_scan_param(3)

print(" first_tr:", file=f)

print_scan_param(1)

print(" last_tr:", file=f)

print_scan_param(2)

C-PAC/CPAC/utils/extract_data_multiscan.py

Line 304 in 25ea215

logger.info("site for sub %s -> %s", sub, subject_map.get(sub))

replicates the original behavior, but we could print the line to the file instead if that was what we wanted to do.

shnizzedy · 2024-02-17T03:01:10Z

CPAC/utils/ndmg_utils.py

@@ -183,8 +189,9 @@ def cor_graph(self, timeseries, attr=None):
        """
        import numpy as np

-        timeseries[0]
+        ts = timeseries[0]  # noqa: F841


This ts isn't used anywhere. Do we care to assign it just to annotate, or should we drop this line?

It's fine for now. Your comments with idea stubs in this PR is a pretty nice list, so we can revisit this when we revisit the other ideas as well.

shnizzedy · 2024-02-20T15:17:58Z

Oh, I forgot to mention this in the PR message, but updating the Function nodes reminded me of something I wondered ages ago, then forgot about:

Why don't we default

C-PAC/CPAC/utils/interfaces/function/function.py

Line 163 in 26ebd1e

as_module=False,

as True (or even not make it an option, and just always have Function nodes "as module"s)?

I.e., when would we want a Function node not be "as module"?

sgiavasis · 2024-02-28T22:48:03Z

Oh, I forgot to mention this in the PR message, but updating the Function nodes reminded me of something I wondered ages ago, then forgot about:

Why don't we default

C-PAC/CPAC/utils/interfaces/function/function.py

Line 163 in 26ebd1e

as_module=False,

as True (or even not make it an option, and just always have Function nodes "as module"s)?
I.e., when would we want a Function node not be "as module"?

This is ancient; I don't remember right now but again, can add to the list of things to revisit in this PR.

shnizzedy · 2024-02-29T19:59:33Z

[This] is ancient; I don't remember right now but again, can add to the list of things to revisit in this PR.

― #2062 (comment)

It's cool; as far as I can tell, it

Includes the variables available in the namespace like if you imported the function normally outside of nipype (so you don't need the imports param or the imports defined in the function itself
Includes the module name in the log instead of just the text of the function

But I don't know how much overhead this costs or saves, or what other considerations we should have about whether / when to use it

mgxd and others added 11 commits August 9, 2017 17:04

ref: isolate working with functions to avoid __future__ conflicts

d9ec592

Merge pull request #2093 from mgxd/fix/py2functions

ce9517a

fix: python 2 Function interfaces recompatibility

Revert "fix: python 2 Function interfaces recompatibility"

295f41a

Merge pull request #2148 from nipy/revert-2093-fix/py2functions

9408fa7

Revert "fix: python 2 Function interfaces recompatibility"

ref: isolate function handling from __future__ imports

6f20051

Merge remote-tracking branch 'nipy/master'

80e41e8

STY: correction of E302 - expected 2 blank lines, found 1

8c39ecd

Merge pull request #2371 from satra/sty/pepping

e28c80a

Sty/pepping

RF: Remove builtins

08a5346

STY: Black

b476758

Merge pull request #3096 from effigies/sty/black

b55e0f7

STY: Black

shnizzedy mentioned this pull request Feb 7, 2024

✅ Fix tests broken by #2058 #2059

Merged

15 tasks

shnizzedy added 5 commits February 12, 2024 20:47

⏪ Restore print statements

9b4ce66

🎨 Replace print with logger.info

380cbed

🎨 Replace print with logger.info

1ccca09

🎨 Replace print with logger.info

fbd0133

🎨 Replace print with logger.info

e5a6f32

shnizzedy changed the base branch from initial-run/ruff to lint/pep8-boolean-comparison February 13, 2024 03:05

shnizzedy added 7 commits February 12, 2024 22:08

🎨 Remove duplicate import

8464bf4

🎨 Replace print with logger.* or raise Exception

82a34ca

⏪ Restore print statement

985ff0c

🎨 Replace print with logger.* or raise Exception

3c8eb24

🎨 Standardize nibabel as nib

46705df

(some were `nibabel as nb`)

🎨 Replace print with logger.* or raise Exception

8f368de

🎨 Replace print with logger.* or raise Exception

43f5f46

shnizzedy force-pushed the lint/print-to-log branch from 91bb731 to 43f5f46 Compare February 14, 2024 05:00

shnizzedy added 4 commits February 14, 2024 18:24

🎨 Replace print with logger.* or raise Exception

2327adf

🎨 Replace print with logger.* or raise Exception

1d79046

🎨 Remove unnecessary spaces between strings

f6651ed

Where reformatting changed things like > ("line 1" > " line 2") to like > "line 1" " line 2" , change to like > "line 1 line 2"

🚨 Lint for 1.8.7

d568009

shnizzedy added 8 commits February 15, 2024 14:58

✨ Automatically assign logger and iflogger variables in function nodes

6ef4d88

✅ Add tests for autoassignment of logger and iflogger

4dd4f15

✅ Test both as_module and not

52ac149

✅ Update sig_imports test to include autologging

2f13f67

:twisted_rightwards_arrowa: Merge nipype/utils/functions.py from ni…

30a24d3

…pype 1.8.6

🚚 Move create_function_from_source from nipype to CPAC

5d28e63

♻️ Clarify as_module imports

da58b7f

👔 Import full module when Function(as_module=True)

25ea215

shnizzedy commented Feb 17, 2024

View reviewed changes

shnizzedy marked this pull request as ready for review February 19, 2024 19:58

shnizzedy force-pushed the lint/print-to-log branch from e460f7d to 26ebd1e Compare February 20, 2024 15:16

shnizzedy force-pushed the lint/print-to-log branch from 26ebd1e to 56930df Compare February 20, 2024 15:23

♻️ Import global Nipype loggers

85737a1

shnizzedy force-pushed the lint/print-to-log branch from 56930df to 85737a1 Compare February 21, 2024 02:20

shnizzedy self-assigned this Feb 21, 2024

shnizzedy added the style label Feb 21, 2024

sgiavasis merged commit 4c4c0fd into lint/pep8-boolean-comparison Feb 28, 2024
43 checks passed

shnizzedy mentioned this pull request Mar 20, 2024

🎨 Run pre-commit hooks on whole repo #2058

Merged

8 tasks

shnizzedy deleted the lint/print-to-log branch May 7, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎨 `print` statements → `log` statements #2062

🎨 `print` statements → `log` statements #2062

shnizzedy commented Feb 7, 2024 •

edited

Loading

shnizzedy left a comment •

edited

Loading

shnizzedy Feb 17, 2024

shnizzedy Feb 17, 2024

shnizzedy Feb 17, 2024

shnizzedy Feb 17, 2024

sgiavasis Feb 28, 2024

shnizzedy commented Feb 20, 2024

sgiavasis commented Feb 28, 2024

shnizzedy commented Feb 29, 2024

	"from CPAC.utils.monitoring.custom_logging import FMLOGGER, IFLOGGER, UTLOGGER,"
	" WFLOGGER"

	def faux_fxn(_loggers: bool = True):
	"""Require autoassignment (for testing)."""
	if _loggers:
	return WFLOGGER, IFLOGGER # noqa: F821
	return luigi_mario # noqa: F821


	@mark.parametrize("as_module", [True, False])
	def test_autologger(as_module: bool) -> None:
	"""Test autoassignment of global Nipype loggers`."""
	interface = Function(
	function=faux_fxn, input_names=["_loggers"], as_module=as_module
	)
	interface.inputs._loggers = False
	with raises(NameError) as name_error:
	interface.run()
	assert "name 'luigi_mario' is not defined" in str(name_error.value)

	interface = Function(
	function=faux_fxn,
	input_names=["_loggers"],
	output_names=["logger", "iflogger"],
	as_module=as_module,
	)
	interface.inputs._loggers = True
	res = interface.run()
	assert res.outputs.logger.name == "nipype.workflow"
	assert res.outputs.iflogger.name == "nipype.interface"

		@@ -175,6 +178,10 @@ def gather_nifti_globs(pipeline_output_folder, resource_list, pull_func=False):
		# remove any extra /'s

	# remove any extra /'s
	pipeline_output_folder = pipeline_output_folder.rstrip("/")

	logger.info(
	"\n\nGathering the output file paths from %s...", pipeline_output_folder
	)

	# this is just to keep the fsl feat config file derivative_list entries
	# nice and lean
	dirs_to_grab = []
	for derivative_name in derivative_list:
	for resource_name in resource_list:
	if resource_name in derivative_name:
	dirs_to_grab.append(derivative_name)

	print("site for sub", sub, "->", subject_map.get(sub))
	print(" scan_parameters: ", file=f)
	print(" tr:", file=f)
	print_scan_param(4)
	print(" acquisition:", file=f)
	print_scan_param(0)
	print(" reference:", file=f)
	print_scan_param(3)
	print(" first_tr:", file=f)
	print_scan_param(1)
	print(" last_tr:", file=f)
	print_scan_param(2)

🎨 print statements → log statements #2062

🎨 print statements → log statements #2062

Conversation

shnizzedy commented Feb 7, 2024 • edited Loading

Fixes

Description

Technical details

Tests

Checklist

Developer Certificate of Origin

shnizzedy left a comment • edited Loading

Choose a reason for hiding this comment

shnizzedy Feb 17, 2024

Choose a reason for hiding this comment

shnizzedy Feb 17, 2024

Choose a reason for hiding this comment

shnizzedy Feb 17, 2024

Choose a reason for hiding this comment

shnizzedy Feb 17, 2024

Choose a reason for hiding this comment

sgiavasis Feb 28, 2024

Choose a reason for hiding this comment

shnizzedy commented Feb 20, 2024

sgiavasis commented Feb 28, 2024

shnizzedy commented Feb 29, 2024

🎨 `print` statements → `log` statements #2062

🎨 `print` statements → `log` statements #2062

shnizzedy commented Feb 7, 2024 •

edited

Loading

shnizzedy left a comment •

edited

Loading