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

Fix Crash when Calculation of size of deleted dirs is asked for #143

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

Atticus1806
Copy link
Contributor

When calling the current code it crashes with:

Calculate size of affected directories? (Y/n): 
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[2], line 1
----> 1 tk.cleaner.cleanup_unused(mode="dryrun")

File ~/src/sisyphus/sisyphus/cleaner.py:316, in cleanup_unused(load_from, job_dirs, mode)
    314     job_dirs = list_all_graph_directories()
    315 to_remove = search_for_unused(job_dirs, verbose=True)
--> 316 remove_directories(to_remove, 'Not used in graph', mode=mode, force=False)

File ~/src/sisyphus/sisyphus/cleaner.py:235, in remove_directories(dirs, message, move_postfix, mode, force)
    233 with tempfile.NamedTemporaryFile() as tmp_file:
    234     for directory in dirs:
--> 235         tmp_file.write(directory + "\x00")
    236     tmp_file.flush()
    237     command = 'du -sch --files0-from=%s' % tmp_file.name

File /usr/lib/python3.10/tempfile.py:622, in _TemporaryFileWrapper.__getattr__.<locals>.func_wrapper(*args, **kwargs)
    620 @_functools.wraps(func)
    621 def func_wrapper(*args, **kwargs):
--> 622     return func(*args, **kwargs)

TypeError: a bytes-like object is required, not 'str'

This fixes this error. Tbh. I am not sure how this has worked before?

@Atticus1806
Copy link
Contributor Author

Currently using python 3.10, at first I thought the default was maybe changed but according to this the default is the same in 3.6:
https://docs.python.org/3.6/library/tempfile.html

Comment on lines +233 to 235
with tempfile.NamedTemporaryFile(mode="w") as tmp_file:
for directory in dirs:
tmp_file.write(directory + "\x00")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if this is the correct solution. Here we write a list of directories to a file. The directories should be delimited with zero-bytes "\x00" to work with the option --files0-from (cf. https://man7.org/linux/man-pages/man1/du.1.html)

If I see it correctly, the default mode is binary write and you change it to text write. With your proposal I think the lines are delimited with the characters "\x00". So then the du command below should not find the corresponding folders?

I think rather should we change what is written to binary:

Suggested change
with tempfile.NamedTemporaryFile(mode="w") as tmp_file:
for directory in dirs:
tmp_file.write(directory + "\x00")
with tempfile.NamedTemporaryFile() as tmp_file:
for directory in dirs:
tmp_file.write(directory.encode() + b"\x00")

(But please test both variants)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so with my variant I got some result (which is hard to confirm whether correct but looked reasonable)
will test yours now

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that \x will make the next two characters be interpreted as a binary value. So your solution works fine.

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 just tested your variant aswell, both produce the same results. Feel free to merge whatever you prefer.

Comment on lines +233 to 235
with tempfile.NamedTemporaryFile(mode="w") as tmp_file:
for directory in dirs:
tmp_file.write(directory + "\x00")
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that \x will make the next two characters be interpreted as a binary value. So your solution works fine.

@michelwi michelwi merged commit 454943a into master Aug 2, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants