overwrite=True ignored in FITS writing for Path filename

Description

Something is wrong in the FITS table writer when providing the input filename as a Path object and overwriting an existing file. Even though the function call clearly has overwrite=True the code fails to overwrite. When the filename is instead a str then the expected behavior is seen. I also don't see this behavior in io.ascii.

Expected behavior

Existing file is overwritten.

How to Reproduce

In [11]: %astro
astropy=6.0.dev673+gd57101dbc9

In [12]: t = Table({"a": [1,2], "b": [3,4]})

In [13]: from pathlib import Path

In [14]: t.write(Path("junk.fits"), format="fits", overwrite=True)

In [15]: t.write(Path("junk.fits"), format="fits", overwrite=True)
---------------------------------------------------------------------------
OSError                                   Traceback (most recent call last)
Cell In[15], line 1
----> 1 t.write(Path("junk.fits"), format="fits", overwrite=True)

File ~/git/astropy/astropy/table/connect.py:130, in TableWrite.__call__(self, serialize_method, *args, **kwargs)
    128 instance = self._instance
    129 with serialize_method_as(instance, serialize_method):
--> 130     self.registry.write(instance, *args, **kwargs)

File ~/git/astropy/astropy/io/registry/core.py:382, in UnifiedOutputRegistry.write(self, data, format, *args, **kwargs)
    377     format = self._get_valid_format(
    378         "write", data.__class__, path, fileobj, args, kwargs
    379     )
    381 writer = self.get_writer(format, data.__class__)
--> 382 return writer(data, *args, **kwargs)

File ~/git/astropy/astropy/io/fits/connect.py:448, in write_table_fits(input, output, overwrite, append)
    446     fits_append(output, table_hdu.data, table_hdu.header, verify=False)
    447 else:
--> 448     table_hdu.writeto(output)

File ~/git/astropy/astropy/io/fits/hdu/base.py:1599, in ExtensionHDU.writeto(self, name, output_verify, overwrite, checksum)
   1596 from .image import PrimaryHDU
   1598 hdulist = HDUList([PrimaryHDU(), self])
-> 1599 hdulist.writeto(name, output_verify, overwrite=overwrite, checksum=checksum)

File ~/git/astropy/astropy/io/fits/hdu/hdulist.py:1010, in HDUList.writeto(self, fileobj, output_verify, overwrite, checksum)
   1006 mode = FILE_MODES[fileobj_mode(fileobj)] if isfile(fileobj) else "ostream"
   1008 # This can accept an open file object that's open to write only, or in
   1009 # append/update modes but only if the file doesn't exist.
-> 1010 fileobj = _File(fileobj, mode=mode, overwrite=overwrite)
   1011 hdulist = self.fromfile(fileobj)
   1012 try:

File ~/git/astropy/astropy/io/fits/file.py:217, in _File.__init__(self, fileobj, mode, memmap, overwrite, cache, use_fsspec, fsspec_kwargs)
    215     self._open_fileobj(fileobj, mode, overwrite)
    216 elif isinstance(fileobj, (str, bytes)):
--> 217     self._open_filename(fileobj, mode, overwrite)
    218 else:
    219     self._open_filelike(fileobj, mode, overwrite)

File ~/git/astropy/astropy/io/fits/file.py:615, in _File._open_filename(self, filename, mode, overwrite)
    613 """Open a FITS file from a filename string."""
    614 if mode == "ostream":
--> 615     self._overwrite_existing(overwrite, None, True)
    617 if os.path.exists(self.name):
    618     with open(self.name, "rb") as f:

File ~/git/astropy/astropy/io/fits/file.py:498, in _File._overwrite_existing(self, overwrite, fileobj, closed)
    496         os.remove(self.name)
    497 else:
--> 498     raise OSError(NOT_OVERWRITING_MSG.format(self.name))

OSError: File junk.fits already exists. If you mean to replace it then use the argument "overwrite=True".
.

Versions

macOS-10.16-x86_64-i386-64bit
Python 3.10.10 (main, Mar 21 2023, 13:41:39) [Clang 14.0.6 ]
astropy 6.0.dev673+gd57101dbc9
Numpy 1.24.2
pyerfa 2.0.0.3
Scipy 1.10.1
Matplotlib 3.7.1

I think the line at https://github.com/astropy/astropy/blob/main/astropy/io/fits/connect.py#L438 may be the issue here. It processes "overwrite" flag only for "str" types but in this case, "output" is a "Path" object.

@draghuram

@draghuram - that's definitely one problem. Just looking at the code it seems there are more issues:

If overwrite and append are both True then it will remove output before appending. I would not expect that since technically appending could be considered a kind of overwriting.
In general removing the existing file before successfully writing the new one is not great. If the write fails for some reason then the original is lost. Ideally it should write to a temp file and then move to the output name.

Agreed. It would be ideal for the overwrite processing to be atomic. I can take a look and submit PR if that is ok. Also, there should be a test for this functionality.

I have to do more testing but the following diff seems to work based on testing few combinations:

     # Check if output file already exists
-    if isinstance(output, str) and os.path.exists(output):
-        if overwrite:
-            os.remove(output)
-        elif not append:
-            raise OSError(NOT_OVERWRITING_MSG.format(output))
 
     if append:
         # verify=False stops it reading and checking the existing file.
         fits_append(output, table_hdu.data, table_hdu.header, verify=False)
     else:
-        table_hdu.writeto(output)
+        table_hdu.writeto(output, overwrite=overwrite)

If both "overwrite" and "append" are specified, "append" takes precedence which I think is appropriate. Also,"overwrite" flag is passed down to "BinTableHDU". Still need to verify whether its implementation is atomic or not.

It seems that the "overwrite" implementation of "BinTableHDU" is at https://github.com/astropy/astropy/blob/main/astropy/io/fits/file.py#L480 and is not atomic. I think it would be best to treat these two issues separately:

Fix the problem of "overwrite=True" getting ignored for "Path"s. Also make sure if both overwrite and append are set, overwrite is ignored.
Make 'overwrite" implementation atomic.

I verified that my diff above works for (1) but need to add automated tests. I don't see any for "Path" objects in the file test_table.py.

Please let me know if I am on right track here.

As per this line:
https://github.com/astropy/astropy/blob/main/astropy/io/fits/tests/test_connect.py#L343

it seems that when both overwrite and append are specified, "append" is ignored. Do we need to keep this behavior or should it be changed so that "overwrite" is ignore instead?

In general removing the existing file before successfully writing the new one is not great. If the write fails for some reason then the original is lost. Ideally it should write to a temp file and then move to the output name.

But the same is done is bit everywhere in io, so maybe worth another issue to find a common solution.

Can we keep this ticket about fixing the immediate issue with pathlib, and maybe append vs overwrite (with should be exclusive).

I agree. It would be best to use this issue to fix the immediate problem without changing anything else. I am trying to add tests that reliably reproduce the error and then I will add the fix.

I've run the following code on main

from astropy.table import Table
from pathlib import Path

t = Table({"a": [1,2], "b": [3,4]})

tmp_dir = Path("tempdir")
tmp_dir.mkdir(exist_ok=True)
filename = Path(f"{tmp_dir}/temp.fits")
print(type(filename))
t.write(filename, format="fits", overwrite=True)
t.write(filename, format="fits", overwrite=True)

filename = Path("temp.fits")
print(type(filename))

t.write(filename, format="fits", overwrite=True)
t.write(filename, format="fits", overwrite=True)

with the following output

<class 'pathlib.PosixPath'>
<class 'pathlib.PosixPath'>
Traceback (most recent call last):
  File "/home/cmarmo/software/astropy/../tests/15350-astropy.py", line 16, in <module>
    t.write(filename, format="fits", overwrite=True)
  File "/home/cmarmo/software/astropy/astropy/table/connect.py", line 130, in __call__
    self.registry.write(instance, *args, **kwargs)
  File "/home/cmarmo/software/astropy/astropy/io/registry/core.py", line 386, in write
    return writer(data, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/cmarmo/software/astropy/astropy/io/fits/connect.py", line 461, in write_table_fits
    table_hdu.writeto(output)
  File "/home/cmarmo/software/astropy/astropy/io/fits/hdu/base.py", line 1571, in writeto
    hdulist.writeto(name, output_verify, overwrite=overwrite, checksum=checksum)
  File "/home/cmarmo/software/astropy/astropy/io/fits/hdu/hdulist.py", line 1033, in writeto
    fileobj = _File(fileobj, mode=mode, overwrite=overwrite)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/cmarmo/software/astropy/astropy/io/fits/file.py", line 218, in __init__
    self._open_filename(fileobj, mode, overwrite)
  File "/home/cmarmo/software/astropy/astropy/io/fits/file.py", line 640, in _open_filename
    self._overwrite_existing(overwrite, None, True)
  File "/home/cmarmo/software/astropy/astropy/io/fits/file.py", line 523, in _overwrite_existing
    raise OSError(NOT_OVERWRITING_MSG.format(self.name))
OSError: File temp.fits already exists. If you mean to replace it then use the argument "overwrite=True".

It looks like for some reason a pathlib.PosixPath filename can be casted to a string when it is composed of more than one pathsegment.

this is "just" a side effect from _expand_user_in_args

astropy/astropy/io/registry/core.py

Line 14 in 45ff837

def _expand_user_in_args(args):

in the first case it converts the pathlib object to a string, in the second it does not because there is no directory.

side effect

Hmm does this mean we also have to patch _expand_user_in_args?

Also...

one simple option would be to change _expand_user_in_args so it always returns the same type it received

diff --git a/astropy/io/registry/core.py b/astropy/io/registry/core.py
index b7325c4beb..3c0e1fad25 100644
--- a/astropy/io/registry/core.py
+++ b/astropy/io/registry/core.py
@@ -15,7 +15,7 @@ def _expand_user_in_args(args):
     # Conservatively attempt to apply `os.path.expanduser` to the first
     # argument, which can be either a path or the contents of a table.
     if len(args) and isinstance(args[0], PATH_TYPES):
-        ex_user = os.path.expanduser(args[0])
+        ex_user = type(args[0])(os.path.expanduser(args[0]))
         if ex_user != args[0] and os.path.exists(os.path.dirname(ex_user)):
             args = (ex_user,) + args[1:]
     return args

(my patch actually breaks 7 tests on its own, so, probably not a good idea)

I completed my patch in #17552 (comment)

Navigation Menu

Repository navigation

`overwrite=True` ignored in FITS writing for Path filename #15350

Description

Expected behavior

How to Reproduce

Versions

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

overwrite=True ignored in FITS writing for Path filename #15350

Description

Description

Expected behavior

How to Reproduce

Versions

Activity

draghuram commented on Sep 19, 2023

taldcroft commented on Sep 19, 2023

draghuram commented on Sep 19, 2023

draghuram commented on Sep 19, 2023

draghuram commented on Sep 20, 2023

draghuram commented on Sep 20, 2023

saimn commented on Sep 21, 2023

draghuram commented on Sep 21, 2023

cmarmo commented on Dec 18, 2024

saimn commented on Dec 18, 2024

pllim commented on Dec 18, 2024

neutrinoceros commented on Dec 19, 2024

neutrinoceros commented on Dec 19, 2024

neutrinoceros commented on Dec 19, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`overwrite=True` ignored in FITS writing for Path filename #15350