Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Add support for .nniignore #2454

Merged
merged 23 commits into from
Jun 19, 2020
Merged

Add support for .nniignore #2454

merged 23 commits into from
Jun 19, 2020

Conversation

ultmaster
Copy link
Contributor

@ultmaster ultmaster commented May 19, 2020

Design:

  • Code dir validation is moved to nnictl.
  • .nniignore is first checked, if it doesn't exist, then fallback to .gitignore.
  • .git folder is still not ignored. If users intend to ignore this folder, use .nniignore.
  • Total file size limit is 300MB, file count limit is 2000.

Future work:

  • nnictl should prepare a snapshot, nni experiment should have the option to use the snapshot directly, instead of using a codeDir.
  • Let's move snapshot to another discussion.

TODO:

  • Documentation

@ultmaster ultmaster closed this May 19, 2020
@ultmaster ultmaster reopened this May 19, 2020
@liuzhe-lz
Copy link
Contributor

I don't think we should silently follow .gitignore. It could be confusing.
I prefer to provide an option to explicitly declare the directory is managed by git. Then we can git ls-tree to determine files to be uploaded, or use git archive HEAD to create snapshot.

@ultmaster
Copy link
Contributor Author

ultmaster commented May 19, 2020

I don't think we should silently follow .gitignore. It could be confusing.
I prefer to provide an option to explicitly declare the directory is managed by git. Then we can git ls-tree to determine files to be uploaded, or use git archive HEAD to create snapshot.

The other tools have two different options:

  • AzureML: look for .amlignore, then .gitignore (like I did)
  • Philly tools: look for .ptignore, then give up

I'm fine with both of them. If you are more comfortable with option 2, let's go ahead with option 2.

As for implementation, I tried using git ls-files, which is very convenient and comes with all the benefits with git (including .gitignores in nested directory), but it cannot use files other than files called .gitignore, which is not acceptable because sometimes the codeDir might contain some small-sized data that is nice to have in experiments but should be kept away from git repository.

@liuzhe-lz
Copy link
Contributor

There could be scenarios like the user copies a 3rd-party library, which is git-ignored, into codeDir and they will get confused when the experiment fails.
So I suggest either not to support gitignore, or to use an explicit option.

@ultmaster
Copy link
Contributor Author

There could be scenarios like the user copies a 3rd-party library, which is git-ignored, into codeDir and they will get confused when the experiment fails.
So I suggest either not to support gitignore, or to use an explicit option.

Okay, I can remove the gitignore part. But first, let's discuss it in tomorrow's meeting, to see if there are any other concerns.

export function* listDirWithIgnoredFiles(root: string, relDir: string, ignoreFiles: string[]): Iterable<string> {
let ignoreFile = undefined;
const source = path.join(root, relDir);
if (fs.existsSync(path.join(source, '.nniignore'))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

will check .nniignore file in every subfolder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Works like .gitignore.

@chicm-ms chicm-ms merged commit 16fd8a2 into master Jun 19, 2020
@liuzhe-lz liuzhe-lz deleted the dev-nniignore branch July 17, 2020 11:17
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.

4 participants