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

Reduce Database Overwriting #73

Closed
wants to merge 1 commit into from
Closed

Reduce Database Overwriting #73

wants to merge 1 commit into from

Conversation

onufer
Copy link
Member

@onufer onufer commented May 14, 2020

DatabaseInterface now checks if the database file that it is
creating already exists and raises an error if it does. This
change is to prevent unfortunate overwrite of an old database.

creating already exists and raises an error if it does. This
change is to prevent the unfortunate overwrite of a database.
@onufer onufer requested a review from ntouran May 14, 2020 22:45
@youngmit
Copy link
Contributor

How do we protect users from overwriting a database that they didn't intend to, without preventing them from overwriting a database that they expected to? One option would be to make this a setting query, which would be silent when submitting a job, or when using --batch, but prompt the user otherwise. Another option is to rename the old database. It is still not immediately obvious to me if that is the right behavior though. I can simply think of too many situations where re-running a case, overwriting a database is the expected behavior.

@onufer
Copy link
Member Author

onufer commented May 15, 2020

I think you are absolutely right that overwriting the database is expected behavior when running/rerunning a case. I think its not expected behavior when you are poking around with interfaces calling interact methods.

Nick and I talked about it a little. One idea I had was when you are calling run or submit (and have db on), ARMI could delete the database (unless the db is also the reloadDB in which case raise an error) since you are running ARMI in the traditional way. You could still still avoiding having interface.interactWhatever methods deleting your stuff when you are poking around with ipython.

Either way Nick said this should go to back burner for now.

@jakehader
Copy link
Member

How's this coming along? Still open?

@onufer
Copy link
Member Author

onufer commented Jun 10, 2020

There were a lot of concerns because the way it is written write now, every run with a pre-existing database, for example when you re-submit a case, would fail.

I offered some solutions, such as when submitting a traditional ARMI run, explicitly deleting the database in a location that is unlikely to be called by custom scripts. This would make custom scripts very unlikely to surprisingly delete a database, but normal runs would continue to function as they have.

Nick requested it be put on the back burner for a while though, as there is a lot of higher priority stuff. He wasn't too excited about any of the solutions either, so its possible/likely it will be abandoned.

@jakehader
Copy link
Member

This PR has been open for sometime and does not look like it's going any further (or maybe the issue was addressed separately). I suggest that you open an issue for the problem and we can revisit later as you said.

@jakehader jakehader closed this May 2, 2021
@john-science john-science deleted the checkDBWrite branch June 7, 2022 18:29
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.

None yet

3 participants