8000 Add a --force command to train by schmmd · Pull Request #1913 · allenai/allennlp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Add a --force command to train #1913

Merged
merged 9 commits into from
Oct 22, 2018
Merged

Add a --force command to train #1913

merged 9 commits into from
Oct 22, 2018

Conversation

schmmd
Copy link
Member
@schmmd schmmd commented Oct 16, 2018

Fixes #1575

if os.path.exists(serialization_dir) and os.listdir(serialization_dir):
if not recover:
if force:
Copy link
Contributor

Choose a reason for hiding this comment

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

The level of nesting that this gets to is scary. I also think you can avoid it - what if you just move the remove block up above, and leave the rest of the code unmodified? That is, have something like:

if recover and force:
    raise ...

if force:
    shutil.rmtree(serialization_dir)

...  # the rest of the code unchanged

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah--I'm fine with that.

@schmmd
Copy link
Member Author
schmmd commented Oct 16, 2018

If I omit the exists check then I need to tell shutil.rmtree to ignore errors, which I would rather avoid.

@matt-gardner
Copy link
Contributor
matt-gardner commented Oct 16, 2018

Yeah, keeping that exists check is fine, I was more commenting on pulling out that branch to up above, not on the specifics of the code block. Looks like you forgot to move back the main logic that you changed, though.

@matt-gardner
Copy link
Contributor

👍

@schmmd schmmd merged commit afc36eb into allenai:master Oct 22, 2018
@schmmd schmmd deleted the force branch October 22, 2018 20:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0