Skip to content

Cross-platform basic implementation of CLI commands - #69

Closed
BramVanroy wants to merge 1 commit into
explosion:v3from
BramVanroy:v3
Closed

Cross-platform basic implementation of CLI commands#69
BramVanroy wants to merge 1 commit into
explosion:v3from
BramVanroy:v3

Conversation

@BramVanroy

Copy link
Copy Markdown

This PR relates to the issue that I brought up here. Not all CLI commands are available on all platforms, so the project.yml is not cross-platform.

In this PR I implement cross-platform helper CLI functions for rm, mkdir, and mv. As you may notice, these are relatively basic and not at all as powerful as native commands. But they do work as a stand-in replacement for the original commands.

This PR can serve as a template for the other projects as well, because here I only update the tagger_parser_ud project but I do not have the time to update all other projects.

@svlandeg

svlandeg commented Aug 11, 2021

Copy link
Copy Markdown
Contributor

Hey Bram,

Thanks for the contribution!

I do agree with you that it would be most ideal if the example projects were working correctly cross-platform. However, the proposed solution here of reimplementing the CLI commands seems a little overkill to me - especially if we'd do this for each and every project separately.

Instead, I think it should be more straightforward to just avoid the commands whenever possible (e.g. spacy train doesn't require an upfront mkdir command for the output), and deal with the rest in custom (pre)processing scripts that just set up the working directory as needed.

@svlandeg svlandeg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some suggestions for this specific project

Comment on lines +42 to +44
- "python utils/file.py mv corpus/${vars.treebank}/${vars.train_name}.spacy corpus/${vars.treebank}/train.spacy"
- "python utils/file.py mv corpus/${vars.treebank}/${vars.dev_name}.spacy corpus/${vars.treebank}/dev.spacy"
- "python utils/file.py mv corpus/${vars.treebank}/${vars.test_name}.spacy corpus/${vars.treebank}/test.spacy"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These mv statements aren't strictly necessary - we could just keep the original train/dev/test names everywhere in the yml file.

Comment on lines +87 to +89
- "python utils/file.py rm training/*"
- "python utils/file.py rm metrics/*"
- "python utils/file.py rm corpus/*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here, we could just have a custom Python script that cleans the correct output directories recursively.

@svlandeg svlandeg added the enhancement New feature or request label Aug 11, 2021

@svlandeg svlandeg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(maintenance note: marking this as "changes requested")

@BramVanroy

Copy link
Copy Markdown
Author

(maintenance note: marking this as "changes requested")

Hi Sofie. I currently do not have time to work on this. Feel free to either modify this PR or close it. As long as the issue explosion/spaCy#6957 remains open, people are free to contribute. Perhaps you can copy-paste your first reply on this PR about how you think this should be implemented to the issue? Curious people looking for the "help wanted" issue tag might then jump in. :-)

@svlandeg

Copy link
Copy Markdown
Contributor

Thanks for the note Bram, and thanks for the contribution as well! Please don't feel discouraged to contribute with more PRs in the future ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants