Skip to content

Split list_topologies.yml into per-model config files#276

Merged
IRDonch merged 3 commits intoopenvinotoolkit:developfrom
IRDonch:split
Aug 15, 2019
Merged

Split list_topologies.yml into per-model config files#276
IRDonch merged 3 commits intoopenvinotoolkit:developfrom
IRDonch:split

Conversation

@IRDonch
Copy link
Copy Markdown

@IRDonch IRDonch commented Aug 5, 2019

Moving to per-model configs will make it easier to maintain the model configuration and reduce the amount of information that needs to be included in the config (since name and subdirectory are now inferred).

This change is transparent to users who don't read list_topologies.yml directly.

For compatibility the -c option is still supported and will let users use a config in the old format. However, it is deprecated and will be removed in a future release. I didn't provide a way to specify an alternate tree of model configs in the new format, because such a feature doesn't seem useful to me (and the config format is still not supposed to be stable).

@IRDonch
Copy link
Copy Markdown
Author

IRDonch commented Aug 5, 2019

Draft because it depends on #267 (well, it doesn't really, but it would make little sense to merge it before that one).

@IRDonch IRDonch marked this pull request as ready for review August 7, 2019 11:25
@snosov1
Copy link
Copy Markdown
Contributor

snosov1 commented Aug 9, 2019

I'd say, so far - so good.

One minor item is that proxy doesn't make it easier to download the models. Things, like pip have a --proxy flag to easily handle this. For downloader I have to invent smth else. Maybe, worth adding such a flag if it's not too big of a hassle.

On another note (we've discussed it briefly with @vladimir-dudnik ) - certain sets of models belong to the same "family" and, maybe, it makes sense to have another level for those. For example, densenet-121/161/169/201. There could be a folder densenet and the respective models inside it. If we talk about intel models, there's action-recognition-0001-decoder/-encoder and others.

Not that I think it's mandatory (even the current state is much better than what we had before) but it somewhat makes things more tidy and the models of the same "family" can share the common documentation.

@IRDonch
Copy link
Copy Markdown
Author

IRDonch commented Aug 9, 2019

One minor item is that proxy doesn't make it easier to download the models. Things, like pip have a --proxy flag to easily handle this. For downloader I have to invent smth else. Maybe, worth adding such a flag if it's not too big of a hassle.

This seems to have nothing to do with this PR, though? And the downloader does support proxies through the standard http_proxy/https_proxy variables.

@snosov1
Copy link
Copy Markdown
Contributor

snosov1 commented Aug 9, 2019

This seems to have nothing to do with this PR, though? And the downloader does support proxies through the standard http_proxy/https_proxy variables.

Correct, just something I stumbled upon when I was trying it out.

What do you think about "family folders" ? Worthwhile or not? If yes, this time or later?

@IRDonch
Copy link
Copy Markdown
Author

IRDonch commented Aug 9, 2019

What do you think about "family folders" ? Worthwhile or not? If yes, this time or later?

It's worth considering; however, it's definitely a task for later, as it would drastically expand the scope of required changes.

@snosov1
Copy link
Copy Markdown
Contributor

snosov1 commented Aug 9, 2019

Got it, thanks!

@kchechil
Copy link
Copy Markdown
Contributor

@yssemaev any update on the question?

@yssemaev
Copy link
Copy Markdown
Contributor

"Copyright (c) 2019 Intel Corporation" header should be in all human readable files

@IRDonch
Copy link
Copy Markdown
Author

IRDonch commented Aug 12, 2019

@yssemaev Do you mean just the copyright line, or the full banner ("Licensed under the Apache License", etc.)?

Roman Donchenko added 3 commits August 14, 2019 16:38
Moving to per-model configs will make it easier to maintain
the model configuration and reduce the amount of information that needs
to be included in the config (since name and subdirectory are now inferred).

This change is transparent to users who don't read list_topologies.yml directly.

For compatibility the -c option is still supported and will let
users use a config in the old format. However, it is deprecated and will be
removed in a future release. I didn't provide a way to specify an alternate
tree of model configs in the new format, because such a feature doesn't seem
useful to me (and the config format is still not supposed to be stable).
…rinting

This has two beneficial effects:

* the output of --print_all is now grouped by category (intel/public);
* the models are now downloaded/converted in the same order they're printed
  by --print_all.
@IRDonch IRDonch merged commit d959cec into openvinotoolkit:develop Aug 15, 2019
@IRDonch IRDonch deleted the split branch August 15, 2019 09:35
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.

5 participants