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

Upgrade python syntax to 3.9 and newer #6376

Merged
merged 1 commit into from Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions Applications/SlicerApp/Testing/Python/CLISerializationTest.py
Expand Up @@ -70,9 +70,9 @@ def deserializeCLI(self, cli_name, json_file_path, parameters=[]):
temp_dir = os.path.expanduser(getattr(args, "/path/to/temp_dir"))

# Create input/output
serializeSeedsOutFile = '%s/%s.acsv' % (temp_dir, 'SeedsSerialized')
deserializeSeedsOutFile = '%s/%s.acsv' % (temp_dir, 'SeedsDeSerialized')
json_file = '%s/%s.json' % (temp_dir, 'ExecutionModelTourSerialized')
serializeSeedsOutFile = f'{temp_dir}/SeedsSerialized.acsv'
deserializeSeedsOutFile = f'{temp_dir}/SeedsDeSerialized.acsv'
json_file = f'{temp_dir}/ExecutionModelTourSerialized.json'

# Copy .mrml file to prevent modification of source tree
mrml_source_path = os.path.join(data_dir, 'ExecutionModelTourTest.mrml')
Expand Down
2 changes: 1 addition & 1 deletion Base/Python/slicer/ScriptedLoadableModule.py
Expand Up @@ -299,7 +299,7 @@ class ScriptedLoadableModuleTest(unittest.TestCase):
"""

def __init__(self, *args, **kwargs):
super(ScriptedLoadableModuleTest, self).__init__(*args, **kwargs)
super().__init__(*args, **kwargs)

# See https://github.com/Slicer/Slicer/pull/6243#issuecomment-1061800718 for more information.
# Do not pass *args, **kwargs since there is no base class after `unittest.TestCase`. This is only relevant to
Expand Down
4 changes: 2 additions & 2 deletions Base/Python/slicer/util.py
Expand Up @@ -2716,7 +2716,7 @@ def _messageDisplay(logLevel, text, testingReturnValue, mainWindowNeeded=False,
if not windowTitle:
windowTitle = slicer.app.applicationName + " " + logLevelString
if slicer.app.testingEnabled():
logging.info("Testing mode is enabled: Returning %s and skipping message box [%s]." % (testingReturnValue, windowTitle))
logging.info(f"Testing mode is enabled: Returning {testingReturnValue} and skipping message box [{windowTitle}].")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to introduce usage of f-strings in logging functions, because using f-strings results in performing expensive string manipulations even if the message does not end up being printed due to the current log level.

Has pyupgrade recommended this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes pyupgrade changes it to .format() and then re-running changes it again to an f-string. The maintainer points to asottile/pyupgrade#503 (comment) in that f-string usage here is not a performance drop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a performance concern fstrings and format statements should be avoided in logging calls since the arguments will be evaluated before the logging method is invoked, so the computation will always happen regardless of log level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to argue with the maintainer of pyupgrade on this one, so since there are no exceptions to keep % formatting only for logging statements with pyupgrade that means I would need to remove pyupgrade from the pre-commit-config.yaml in this PR and we would have to apply all the changes in a manual review basis and every so often update the Slicer code base to get back into the compliance that we believe in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link @jamesobutler, it led to some very interesting discussions. There seems to be a very strong disagreement between various Python developers on this topic and probably the correct answer depends on the use case. The insignificant performance drop was a measurement error, in the same issue further along the discussion, the actual performance drop was shown to be about 20%. So, this is a real problem. There are additional potential issues with exceptions during string conversion.

However, in our specific these cases here, there is no performance or exception-safety problems, so it should be fine to use f-strings.

My only concern is that using f-strings in logging would make this the default choice everywhere, even where performance matters. But maybe keeping the classic formatting in our log messages is not a very effective way of educating/reminding developers anyway, and if the cost is not to use pyupgrade at all or spend a lot of time with fighting with various lint tools, then it may just not worth it.

So, I'm OK with keeping the suggested changes.

Copy link
Member

@jcfr jcfr Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at https://pylint.pycqa.org/en/latest/user_guide/messages/warning/logging-format-interpolation.html, the following is indicated:

Another reasonable option is to use f-string. If you want to do that, you need to enable logging-format-interpolation and disable logging-fstring-interpolation.

Given the fact there is no significant performance impact 1, enforcing f-string everywhere is reasonable.

That said, I think we should explicitly configure pylint with:

  • logging-fstring-interpolation -> disabled
  • logging-format-interpolation -> enabled

Footnotes

  1. https://github.com/Slicer/Slicer/pull/6376/files#r984071600

return testingReturnValue
if mainWindowNeeded and mainWindow() is None:
return
Expand All @@ -2739,7 +2739,7 @@ def messageBox(text, parent=None, **kwargs):
import logging, qt, slicer
if slicer.app.testingEnabled():
testingReturnValue = qt.QMessageBox.Ok
logging.info("Testing mode is enabled: Returning %s (qt.QMessageBox.Ok) and displaying an auto-closing message box [%s]." % (testingReturnValue, text))
logging.info(f"Testing mode is enabled: Returning {testingReturnValue} (qt.QMessageBox.Ok) and displaying an auto-closing message box [{text}].")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, I would not use f-strings in logging.

slicer.util.delayDisplay(text, autoCloseMsec=3000, parent=parent, **kwargs)
return testingReturnValue

Expand Down
@@ -1,4 +1,3 @@

import vtk

import slicer
Expand Down
2 changes: 1 addition & 1 deletion Modules/Scripted/DataProbe/DataProbe.py
Expand Up @@ -169,7 +169,7 @@ def getPixelString(self, volumeNode, ijk):
value = self.calculateTensorScalars(tensor, operation=operation)
if value is not None:
valueString = ("%f" % value).rstrip('0').rstrip('.')
return "%s %s" % (scalarVolumeDisplayNode.GetScalarInvariantAsString(), valueString)
return f"{scalarVolumeDisplayNode.GetScalarInvariantAsString()} {valueString}"
else:
return scalarVolumeDisplayNode.GetScalarInvariantAsString()

Expand Down
4 changes: 2 additions & 2 deletions Modules/Scripted/ImportItkSnapLabel/ImportItkSnapLabel.py
Expand Up @@ -26,7 +26,7 @@ def __init__(self, parent):
# (identified by its special name <moduleName>FileReader)
#

class ImportItkSnapLabelFileReader(object):
class ImportItkSnapLabelFileReader:

def __init__(self, parent):
self.parent = parent
Expand Down Expand Up @@ -115,7 +115,7 @@ def parseLabelFile(filename):
colors = []

lineIndex = 0
with open(filename, "r") as fileobj:
with open(filename) as fileobj:
for line in fileobj:
lineIndex += 1
commentLine = commentLineRegex.search(line)
Expand Down
14 changes: 7 additions & 7 deletions Modules/Scripted/WebServer/WebServer.py
Expand Up @@ -299,10 +299,10 @@ def __init__(self, server_address=("", 2016), requestHandlers=None, docroot='.',
self.requestCommunicators = {}
self.enableCORS = enableCORS

class DummyRequestHandler(object):
class DummyRequestHandler:
pass

class SlicerRequestCommunicator(object):
class SlicerRequestCommunicator:
"""
Encapsulate elements for handling event driven read of request.
An instance is created for each client connection to our web server.
Expand Down Expand Up @@ -374,7 +374,7 @@ def onReadable(self, fileno):
self.logMessage('Found end of header with no content, so body is empty')
requestHeader = self.requestSoFar[:-2]
requestComplete = True
except socket.error as e:
except OSError as e:
print('Socket error: ', e)
print('So far:\n', self.requestSoFar)
requestComplete = True
Expand Down Expand Up @@ -475,7 +475,7 @@ def onWritable(self, fileno):
self.response = self.response[sent:]
self.sentSoFar += sent
self.logMessage('sent: %d (%d of %d, %f%%)' % (sent, self.sentSoFar, self.toSend, 100. * self.sentSoFar / self.toSend))
except socket.error as e:
except OSError as e:
self.logMessage('Socket error while sending: %s' % e)
sendError = True

Expand All @@ -493,8 +493,8 @@ def onServerSocketNotify(self, fileno):
fileno = connectionSocket.fileno()
self.requestCommunicators[fileno] = self.SlicerRequestCommunicator(connectionSocket, self.requestHandlers, self.docroot, self.logMessage, self.enableCORS)
self.logMessage('Connected on %s fileno %d' % (connectionSocket, connectionSocket.fileno()))
except socket.error as e:
self.logMessage('Socket Error', socket.error, e)
except OSError as e:
self.logMessage('Socket Error', OSError, e)

def start(self):
"""start the server
Expand Down Expand Up @@ -537,7 +537,7 @@ def findFreePort(self, port=2016):
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
s.bind(("", port))
except socket.error as e:
except OSError as e:
portFree = False
port += 1
finally:
Expand Down
12 changes: 6 additions & 6 deletions Modules/Scripted/WebServer/WebServerLib/DICOMRequestHandler.py
Expand Up @@ -5,7 +5,7 @@
import slicer


class DICOMRequestHandler(object):
class DICOMRequestHandler:
"""
Implements the mapping between DICOMweb endpoints
and ctkDICOMDatabase api calls.
Expand Down Expand Up @@ -104,13 +104,13 @@ def handleStudies(self, parsedURL, requestBody):
firstInstance = instances[0]
dataset = pydicom.dcmread(slicer.dicomDatabase.fileForInstance(firstInstance), stop_before_pixels=True)
studyDataset = pydicom.dataset.Dataset()
studyDataset.SpecificCharacterSet = [u'ISO_IR 100']
studyDataset.SpecificCharacterSet = ['ISO_IR 100']
studyDataset.StudyDate = dataset.StudyDate
studyDataset.StudyTime = dataset.StudyTime
studyDataset.StudyDescription = dataset.StudyDescription if hasattr(studyDataset, 'StudyDescription') else None
studyDataset.StudyInstanceUID = dataset.StudyInstanceUID
studyDataset.AccessionNumber = dataset.AccessionNumber
studyDataset.InstanceAvailability = u'ONLINE'
studyDataset.InstanceAvailability = 'ONLINE'
studyDataset.ModalitiesInStudy = list(modalitiesInStudy)
studyDataset.ReferringPhysicianName = dataset.ReferringPhysicianName
studyDataset[self.retrieveURLTag] = pydicom.dataelem.DataElement(
Expand Down Expand Up @@ -164,10 +164,10 @@ def handleInstances(self, parsedURL, requestBody):
for instance in instances:
dataset = pydicom.dcmread(slicer.dicomDatabase.fileForInstance(instance), stop_before_pixels=True)
instanceDataset = pydicom.dataset.Dataset()
instanceDataset.SpecificCharacterSet = [u'ISO_IR 100']
instanceDataset.SpecificCharacterSet = ['ISO_IR 100']
instanceDataset.SOPClassUID = dataset.SOPClassUID
instanceDataset.SOPInstanceUID = dataset.SOPInstanceUID
instanceDataset.InstanceAvailability = u'ONLINE'
instanceDataset.InstanceAvailability = 'ONLINE'
instanceDataset[self.retrieveURLTag] = pydicom.dataelem.DataElement(
0x00080190, "UR", "http://example.com") # TODO: provide WADO-RS RetrieveURL
instanceDataset.StudyInstanceUID = dataset.StudyInstanceUID
Expand Down Expand Up @@ -219,7 +219,7 @@ def handleSeries(self, parsedURL, requestBody):
firstInstance = instances[0]
dataset = pydicom.dcmread(slicer.dicomDatabase.fileForInstance(firstInstance), stop_before_pixels=True)
seriesDataset = pydicom.dataset.Dataset()
seriesDataset.SpecificCharacterSet = [u'ISO_IR 100']
seriesDataset.SpecificCharacterSet = ['ISO_IR 100']
seriesDataset.Modality = dataset.Modality
seriesDataset.SeriesInstanceUID = dataset.SeriesInstanceUID
seriesDataset.SeriesNumber = dataset.SeriesNumber
Expand Down
Expand Up @@ -397,7 +397,7 @@
import slicer


class SlicerRequestHandler(object):
class SlicerRequestHandler:
"""Implements the Slicer REST api"""

def __init__(self, enableExec=False):
Expand Down Expand Up @@ -810,7 +810,7 @@ def getNRRD(self, volumeID):
supportedScalarTypes = ["short", "double"]
scalarType = imageData.GetScalarTypeAsString()
if scalarType not in supportedScalarTypes:
self.logMessage('Can only get volumes of types %s, not %s' % (str(supportedScalarTypes), scalarType))
self.logMessage(f'Can only get volumes of types {str(supportedScalarTypes)}, not {scalarType}')
self.logMessage('Converting to short, but may cause data loss.')
volumeArray = numpy.array(volumeArray, dtype='int16')
scalarType = 'short'
Expand Down
Expand Up @@ -4,7 +4,7 @@
import re


class StaticPagesRequestHandler(object):
class StaticPagesRequestHandler:
"""Serves static pages content (files) from the configured docroot

uriRewriteRules member variable contain a list of string pairs. iI each pair,
Expand Down Expand Up @@ -71,6 +71,6 @@ def handleRequest(self, method, uri, requestBody):
fp = open(path, 'rb')
responseBody = fp.read()
fp.close()
except IOError:
except OSError:
responseBody = None
return contentType, responseBody
2 changes: 1 addition & 1 deletion Utilities/Scripts/genqrc.py
Expand Up @@ -18,7 +18,7 @@ def writeFile(path, content):
pass

# Write file
with open(path, "wt") as f:
with open(path, "w") as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since text mode (t) is the default 1, this makes sense ✔️

Footnotes

  1. https://stackoverflow.com/questions/23051062/open-files-in-rt-and-wt-modes

f.write(content)


Expand Down