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

run.py avoid writing to disk by default #225

Closed
ilario opened this issue Dec 7, 2022 · 5 comments · Fixed by #227
Closed

run.py avoid writing to disk by default #225

ilario opened this issue Dec 7, 2022 · 5 comments · Fixed by #227

Comments

@ilario
Copy link
Contributor

ilario commented Dec 7, 2022

  • Operating System: Arch Linux
  • Python version: 3.10.8
  • summit version used: 0.8.8

Description

The default location for saving stuff is not writable, on my system.
And the default behavior is to save stuff on disk.
In my opinion, by default, no stuff should be saved, neither temporarily, unless it is actually unavoidable.

This causes this error on my system:

PermissionError                           Traceback (most recent call last)
Cell In[37], line 4
      2 rNMadaptive_20220301 = Runner(strategy=strategyNMadaptive, experiment=emul_constr_20220301, max_iterations=20)
      3 rNMadaptive_20220301.reset()
----> 4 rNMadaptive_20220301.run(prev_res=starting_points_20220301)

File /home/ilario/.venv/lib/python3.10/site-packages/summit/run.py:145, in Runner.run(self, **kwargs)
    143 save_dir = pathlib.Path(save_dir) / "runner" / str(self.uuid_val)
    144 if not os.path.isdir(save_dir):
--> 145     os.makedirs(save_dir)
    146 save_at_end = kwargs.get("save_at_end", True)
    148 n_objs = len(self.experiment.domain.output_variables)

File /usr/lib/python3.10/os.py:215, in makedirs(name, mode, exist_ok)
    213 if head and tail and not path.exists(head):
    214     try:
--> 215         makedirs(head, exist_ok=exist_ok)
    216     except FileExistsError:
    217         # Defeats race condition when another thread created the path
    218         pass

File /usr/lib/python3.10/os.py:215, in makedirs(name, mode, exist_ok)
    213 if head and tail and not path.exists(head):
...
    227     # Cannot rely on checking for EEXIST, since the operating system
    228     # could give priority to other errors like EACCES or EROFS
    229     if not exist_ok or not path.isdir(name):

PermissionError: [Errno 13] Permission denied: '/usr/bin/.summit'

A possible solution is to specify the save_dir path in the run command like:

rNM_20220301.run(prev_res=starting_points_20220301, save_dir="~")

But what I would expect is that if save_freq is unset it does not save any file at all, rather than creating it and deleting it.

In order to do so, the lines to be changed are these ones:

summit/summit/run.py

Lines 144 to 145 in a5e4d0c

if not os.path.isdir(save_dir):
os.makedirs(save_dir)

summit/summit/run.py

Lines 178 to 181 in a5e4d0c

if save_freq is not None:
file = save_dir / f"iteration_{i}.json"
if i % save_freq == 0:
self.save(file)

summit/summit/run.py

Lines 202 to 204 in a5e4d0c

if save_at_end:
file = save_dir / f"iteration_{i}.json"
self.save(file)

summit/summit/run.py

Lines 343 to 344 in a5e4d0c

if not os.path.isdir(save_dir):
os.makedirs(save_dir)

summit/summit/run.py

Lines 402 to 408 in a5e4d0c

if save_freq is not None:
file = save_dir / f"iteration_{i}.json"
if i % save_freq == 0:
self.save(file)
neptune_exp.send_artifact(str(file))
if not save_dir:
os.remove(file)

summit/summit/run.py

Lines 430 to 435 in a5e4d0c

if save_at_end:
file = save_dir / f"iteration_{i}.json"
self.save(file)
neptune_exp.send_artifact(str(file))
if not save_dir:
os.remove(file)

What I Did

python -m venv .venv     
source .venv/bin/activate
pip install -U summit

Open VSCodium and indicate to use .venv as an environment.

Run some summit stuff.

@marcosfelt
Copy link
Member

This is definitely a bug.

The temporary solution to is to specify the save_dir:

rNMadaptive_20220301 = Runner(save_dir=".summit", strategy=strategyNMadaptive, experiment=emul_constr_20220301, max_iterations=20)

I think the correct solution would be to change the Runner.run method default from saving to not saving and more clearly explain what the default saving folder is. Does that make sense?

summit/summit/run.py

Lines 134 to 136 in a5e4d0c

save_at_end : bool, optional
Save the state of the optimization at the end of a run, even if it is stopped early.
Default is True.

@ilario
Copy link
Contributor Author

ilario commented Dec 7, 2022

I think the correct solution would be to change the Runner.run method default from saving to not saving

Yes, I agree this would help :)

Additionally to that, a check should be added here:

summit/summit/run.py

Lines 144 to 145 in a5e4d0c

if not os.path.isdir(save_dir):
os.makedirs(save_dir)

and here:

summit/summit/run.py

Lines 343 to 344 in a5e4d0c

if not os.path.isdir(save_dir):
os.makedirs(save_dir)

But I am not sure about what should be checked, the presence of save_dir?

@marcosfelt
Copy link
Member

Hi @ilario! Do you mind checking out this PR and seeing if it works for you?

git clone https://github.com/sustainable-processes/summit.git
git fetch origin pull/227/head:fix_runner
git checkout fix_runner
python3 -m venv .venv
source .venv/bin/activate
pip install -e .

@ilario
Copy link
Contributor Author

ilario commented Dec 9, 2022

Tested and it works for me (I did test only with and without save_dir, I did not test each possible case) :)

When I installed it with -e I got this error:

$ pip install -e .
Obtaining file:///home/ilario/software/summit
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
ERROR: Project file:///home/ilario/software/summit has a 'pyproject.toml' and its build backend is missing the 'build_editable' hook. Since it does not have a 'setup.py' nor a 'setup.cfg', it cannot be installed in editable mode. Consider using a build backend that supports PEP 660.

but when installing it without -e it just worked :)

@marcosfelt
Copy link
Member

Great! I probably should have written more test cases but it passes all the current ones, so I'm happy with that for now.

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 a pull request may close this issue.

2 participants