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.

Sign up for GitHub

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

Jump to bottom

python: Use a context manager for opening files (SIM115) to solve some ResourceWarnings #4224

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+167 −186
Open

python: Use a context manager for opening files (SIM115) to solve some ResourceWarnings #4224

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
261769d
grass.script.db: Close opened file in db_connection
echoix Aug 24, 2024
1ca0c19
t.unregister: Use a context manager for opening files (SIM115)
echoix Aug 24, 2024
7a98608
t.remove: Use a context manager for opening files (SIM115)
echoix Aug 24, 2024
7963689
t.rast.what: Use a context manager for opening files (SIM115)
echoix Aug 24, 2024
a31c297
grass.temporal.aggregation: Use a context manager for opening files (…
echoix Aug 24, 2024
bb2e999
Apply suggestions from code review
echoix Aug 26, 2024
f7a60ec
Update t.rast.what.py
echoix Aug 26, 2024
7c2df57
Merge branch 'main' into fix-resourcewarning-1
echoix Sep 7, 2024
53e28d7
Merge remote-tracking branch 'origin/main' into fix-resourcewarning-1
echoix Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions python/grass/script/db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,8 @@ def db_connection(force=False, env=None):
:return: parsed output of db.connect
""" # noqa: E501
try:
nuldev = open(os.devnull, "w")
conn = parse_command("db.connect", flags="g", stderr=nuldev, env=env)
nuldev.close()
with open(os.devnull, "w") as nuldev:
conn = parse_command("db.connect", flags="g", stderr=nuldev, env=env)
echoix marked this conversation as resolved.
Show resolved Hide resolved
except CalledModuleError:
conn = None

Expand Down
19 changes: 8 additions & 11 deletions python/grass/temporal/aggregation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,11 @@ def aggregate_raster_maps(

# Create the r.series input file
filename = gs.tempfile(True)
file = open(filename, "w")
with open(filename, "w") as out_file:
for name in inputs:
string = "%s\n" % (name)
out_file.write(string)

for name in inputs:
string = "%s\n" % (name)
file.write(string)

file.close()
# Run r.series
try:
if len(inputs) > 1000:
Expand Down Expand Up @@ -365,11 +363,10 @@ def aggregate_by_topology(
if len(aggregation_list) > 1:
# Create the r.series input file
filename = gs.tempfile(True)
file = open(filename, "w")
for name in aggregation_list:
string = "%s\n" % (name)
file.write(string)
file.close()
with open(filename, "w") as out_file:
for name in aggregation_list:
string = "%s\n" % (name)
out_file.write(string)

mod = copy.deepcopy(r_series)
mod(file=filename, output=output_name)
Expand Down
288 changes: 139 additions & 149 deletions temporal/t.rast.what/t.rast.what.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,78 +373,72 @@ def one_point_per_row_output(
output is of type: x,y,start,end,value
"""
# open the output file for writing
out_file = open(output, "w") if output != "-" else sys.stdout

if write_header is True:
out_str = ""
if vcat:
out_str += "cat{sep}"
if site_input:
out_str += "x{sep}y{sep}site{sep}start{sep}end{sep}value\n"
else:
out_str += "x{sep}y{sep}start{sep}end{sep}value\n"
out_file.write(out_str.format(sep=separator))

for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s" % (file_name)))
map_list = output_time_list[count]
in_file = open(file_name, "r")
for line in in_file:
line = line.split(separator)
with open(output, "w") if output != "-" else sys.stdout as out_file:
if write_header is True:
out_str = ""
if vcat:
cat = line[0]
x = line[1]
y = line[2]
values = line[4:]
if site_input:
site = line[3]
values = line[5:]

out_str += "cat{sep}"
if site_input:
out_str += "x{sep}y{sep}site{sep}start{sep}end{sep}value\n"
else:
x = line[0]
y = line[1]
if site_input:
site = line[2]
values = line[3:]
out_str += "x{sep}y{sep}start{sep}end{sep}value\n"
out_file.write(out_str.format(sep=separator))

for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s" % (file_name)))
map_list = output_time_list[count]
with open(file_name) as in_file:
for line in in_file:
line = line.split(separator)
if vcat:
cat = line[0]
x = line[1]
y = line[2]
values = line[4:]
if site_input:
site = line[3]
values = line[5:]

for i in range(len(values)):
start, end = map_list[i].get_temporal_extent_as_tuple()
if vcat:
cat_str = "{ca}{sep}".format(ca=cat, sep=separator)
else:
cat_str = ""
if site_input:
coor_string = (
"%(x)10.10f%(sep)s%(y)10.10f%(sep)s%(site_name)s%(sep)s"
% (
else:
x = line[0]
y = line[1]
if site_input:
site = line[2]
values = line[3:]

for i in range(len(values)):
start, end = map_list[i].get_temporal_extent_as_tuple()
if vcat:
cat_str = "{ca}{sep}".format(ca=cat, sep=separator)
else:
cat_str = ""
if site_input:
coor_string = (
"%(x)10.10f%(sep)s%(y)10.10f%(sep)s%(site_name)s%(sep)s"
% (
{
"x": float(x),
"y": float(y),
"site_name": str(site),
"sep": separator,
}
)
)
else:
coor_string = "%(x)10.10f%(sep)s%(y)10.10f%(sep)s" % (
{"x": float(x), "y": float(y), "sep": separator}
)
time_string = "%(start)s%(sep)s%(end)s%(sep)s%(val)s\n" % (
{
"x": float(x),
"y": float(y),
"site_name": str(site),
"start": str(start),
"end": str(end),
"val": (values[i].strip()),
"sep": separator,
}
)
)
else:
coor_string = "%(x)10.10f%(sep)s%(y)10.10f%(sep)s" % (
{"x": float(x), "y": float(y), "sep": separator}
)
time_string = "%(start)s%(sep)s%(end)s%(sep)s%(val)s\n" % (
{
"start": str(start),
"end": str(end),
"val": (values[i].strip()),
"sep": separator,
}
)

out_file.write(cat_str + coor_string + time_string)

in_file.close()

if out_file is not sys.stdout:
out_file.close()
Comment on lines -446 to -447
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a way to have a test that triggers this? When reading carefully my code coverage equivalent branch, the t.rast.what file was only imported. I think it's just a collection problem, as there is a test using stdout and doesn't specify layout, so row will be used, entering in this function.
I want to know what happens when having a sys.stdout used as with a context manager, does it close it when exiting the context? It didn't break any tests though.
To be sure of the syntax, in the with of out_file above, I used way more parentheses, and let black simplify them back. I also checked with small examples in the same function above that pyright detected usages that allow or not with a context manager, and sys.stdout can be used with a context manager. It works just like text files. Can it really be closed?

Copy link
Member

Choose a reason for hiding this comment

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

There are three tests in test_what.py which test with output="-", so that's likely testing this stdout case.

I didn't check, but I would guess that you can close stdout, but you don't want to because any code later can be "surprised" by that.

out_file.write(cat_str + coor_string + time_string)


############################################################################
Expand All @@ -460,84 +454,84 @@ def one_point_per_col_output(
Each row represents a single raster map, hence a single time stamp
"""
# open the output file for writing
out_file = open(output, "w") if output != "-" else sys.stdout
# out_file = open(output, "w") if output != "-" else sys.stdout
echoix marked this conversation as resolved.
Show resolved Hide resolved

first = True
for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s" % (file_name)))
map_list = output_time_list[count]
in_file = open(file_name, "r")
lines = in_file.readlines()
with open(output, "w") if output != "-" else sys.stdout as out_file:
Copy link
Member

Choose a reason for hiding this comment

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

Python code:

import sys
with sys.stdout as f:
    f.write("Hello\n")
sys.stdout.write("World\n")

produces:

Hello
Traceback (most recent call last):
  File "/tmp/test.py", line 4, in <module>
    sys.stdout.write("World\n")
ValueError: I/O operation on closed file.

And I think that will be unexpected for anyone who will touch this code later.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not fixed yet, I only solved the conflicts

for count in range(len(output_files)):
file_name = output_files[count]
gs.verbose(_("Transforming r.what output file %s" % (file_name)))
map_list = output_time_list[count]
with open(file_name) as in_file:
lines = in_file.readlines()

matrix = []
for line in lines:
matrix.append(line.split(separator))
matrix = []
for line in lines:
matrix.append(line.split(separator))

num_cols = len(matrix[0])

if first is True:
if write_header is True:
out_str = "start%(sep)send" % ({"sep": separator})
num_cols = len(matrix[0])

# Define different separator for coordinates and sites
if separator == ",":
coor_sep = ";"
else:
coor_sep = ","
if first is True:
if write_header is True:
out_str = "start%(sep)send" % ({"sep": separator})

for row in matrix:
if vcat:
cat = row[0]
x = row[1]
y = row[2]
out_str += (
"{sep}{cat}{csep}{x:10.10f}{csep}"
"{y:10.10f}".format(
cat=cat,
x=float(x),
y=float(y),
sep=separator,
csep=coor_sep,
)
)
if site_input:
site = row[3]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)
# Define different separator for coordinates and sites
if separator == ",":
coor_sep = ";"
else:
x = row[0]
y = row[1]
out_str += "{sep}{x:10.10f}{csep}{y:10.10f}".format(
x=float(x), y=float(y), sep=separator, csep=coor_sep
)
if site_input:
site = row[2]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)
coor_sep = ","

for row in matrix:
if vcat:
cat = row[0]
x = row[1]
y = row[2]
out_str += (
"{sep}{cat}{csep}{x:10.10f}{csep}"
"{y:10.10f}".format(
cat=cat,
x=float(x),
y=float(y),
sep=separator,
csep=coor_sep,
)
)
if site_input:
site = row[3]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)
else:
x = row[0]
y = row[1]
out_str += "{sep}{x:10.10f}{csep}{y:10.10f}".format(
x=float(x), y=float(y), sep=separator, csep=coor_sep
)
if site_input:
site = row[2]
out_str += "{sep}{site}".format(sep=coor_sep, site=site)

out_file.write(out_str + "\n")
out_file.write(out_str + "\n")

first = False
first = False

if vcat:
ncol = 4
else:
ncol = 3
for col in range(num_cols - ncol):
start, end = output_time_list[count][col].get_temporal_extent_as_tuple()
time_string = "%(start)s%(sep)s%(end)s" % (
{"start": str(start), "end": str(end), "sep": separator}
)
out_file.write(time_string)
for row in range(len(matrix)):
value = matrix[row][col + ncol]
out_file.write(
"%(sep)s%(value)s" % ({"sep": separator, "value": value.strip()})
if vcat:
ncol = 4
else:
ncol = 3
for col in range(num_cols - ncol):
start, end = output_time_list[count][col].get_temporal_extent_as_tuple()
time_string = "%(start)s%(sep)s%(end)s" % (
{"start": str(start), "end": str(end), "sep": separator}
)
out_file.write("\n")
out_file.write(time_string)
for row in range(len(matrix)):
value = matrix[row][col + ncol]
out_file.write(
"%(sep)s%(value)s"
% ({"sep": separator, "value": value.strip()})
)
out_file.write("\n")

in_file.close()
if out_file is not sys.stdout:
out_file.close()
in_file.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of hard to review this due to the changed indentation, but shouldn't this be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. At first I wasn't as sure, but in the 3 later PRs (not submitted here yet), I've been doing it, after having tried it more.

Unfortunately, there will be a lots of places in future PRs where the indentation will change. Using side by side comparaison instead of inline would be easier to follow. I am trying to avoid just indenting everything. There are simple cases where a Path(a_file_var).read_text() can be used instead, and I try to use it first.
(See example in https://docs.astral.sh/ruff/rules/read-whole-file/ and https://docs.astral.sh/ruff/rules/write-whole-file/).

Some places I try to just change the order if there are no dependencies so the with is on its own without having to change the whole indent. It's difficult when it's used way below afterwards (like stds_export in a future PR, the diff is a little harder to follow).

Copy link
Member

Choose a reason for hiding this comment

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

What about refactoring some of the functionality out to functions first so that the investment into the review is for the refactoring rather than just with statement? (Probably as a PR separate from this one.)

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm for applying any "read/write whole file" changes first before this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I'm for applying any "read/write whole file" changes first before this PR.

The ones that could be detected automatically were done in #4047 in mid July. The rule is active since so no more new introductions can happen.

It's only after some manual refactorings that I saw new occurrences appear.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about refactoring some of the functionality out to functions first so that the investment into the review is for the refactoring rather than just with statement? (Probably as a PR separate from this one.)

You're aware that there are 368+ places where there's missing a context manager for opened files? It's something a bit big... When the goal is to reduce some new Pylint errors, and help introducing pytest on windows....

Copy link
Member

Choose a reason for hiding this comment

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

Can we make a plan how to handle all those? It will be a lot to review even if the changes would be trivial.

If the edits are automated and 1:1, then we can say we will review only a sample. What I as suggesting before is that we could treat that based on categories: dev/null group, stdout group, pathlib one-liners, huge changes which may as well be refactoring, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not automated, yet. The patterns are easy to apply, so in theory there is nothing that would prevent ruff to create a fix in the future, buts it's just not done yet.

It ends up being a little keyboard dance, so it's not that long to do.

echoix marked this conversation as resolved.
Show resolved Hide resolved


############################################################################
Expand All @@ -554,7 +548,6 @@ def one_point_per_timerow_output(
3730731.49590371|5642483.51236521|6|8|7|7
3581249.04638104|5634411.97526282|5|8|7|7
""" # noqa: E501
out_file = open(output, "w") if output != "-" else sys.stdout

matrix = []
header = ""
Expand All @@ -564,7 +557,6 @@ def one_point_per_timerow_output(
file_name = output_files[count]
gs.verbose("Transforming r.what output file %s" % (file_name))
map_list = output_time_list[count]
in_file = open(file_name, "r")

if write_header:
if first is True:
Expand All @@ -583,7 +575,8 @@ def one_point_per_timerow_output(
)
header += time_string

lines = in_file.readlines()
with open(file_name) as in_file:
lines = in_file.readlines()

for i in range(len(lines)):
cols = lines[i].split(separator)
Expand All @@ -603,26 +596,23 @@ def one_point_per_timerow_output(

first = False

in_file.close()

if write_header:
out_file.write(header + "\n")
with open(output, "w") if output != "-" else sys.stdout as out_file:
if write_header:
out_file.write(header + "\n")

gs.verbose(_("Writing the output file <%s>" % (output)))
for row in matrix:
first = True
for col in row:
value = col.strip()
gs.verbose(_("Writing the output file <%s>" % (output)))
for row in matrix:
first = True
for col in row:
value = col.strip()

if first is False:
out_file.write("%s" % (separator))
out_file.write(value)
if first is False:
out_file.write("%s" % (separator))
out_file.write(value)

first = False
first = False

out_file.write("\n")
if out_file is not sys.stdout:
out_file.close()
out_file.write("\n")


############################################################################
Expand Down
Loading
Loading