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

add model_info_local argument to download function #49

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

add model_info_local argument to download function #49

wants to merge 5 commits into from

Conversation

fcakyon
Copy link

@fcakyon fcakyon commented Jun 24, 2021

Motivation

We want to parse model info from remote when using download function. Fixes #51

Modification

add model_info_local argument to download function.

BC-breaking (Optional)

Previous usage is not affected.

@zhouzaida
Copy link
Collaborator

Thanks for your contribution. But why we need download a checkpoint from remote GitHub.

@fcakyon
Copy link
Author

fcakyon commented Jun 24, 2021

Thanks for your contribution. But why we need download a checkpoint from remote GitHub.

@zhouzaida because model_zoo.yml file may not be available at local.

@zhouzaida
Copy link
Collaborator

got it

@zhouzaida
Copy link
Collaborator

zhouzaida commented Jun 24, 2021

Got it. Please add an unittest. Another thing is that the config file can not be successfully obtained from remote because the config is parsed from the local repo.

@fcakyon
Copy link
Author

fcakyon commented Jun 24, 2021

Got it. Please add an unittest. Another thing is that the config file can not be obtained from remote because the config is parsed from the local repo.

@zhouzaida it is also an issue. wheel package of mmdetection does not include configs directory and mim gives an error when trying to load config from local. I have created a separate issue for that: #50

@fcakyon
Copy link
Author

fcakyon commented Jun 24, 2021

@zhouzaida added tests for model_info_local=False.

@ZwwWayne ZwwWayne requested a review from zhouzaida July 27, 2021 07:12
@zhouzaida
Copy link
Collaborator

hi @fcakyon , the following lines should be moved. In addition, we download checkpoints and configs by default so we can provide two options to only download one of them

if version:
raise ValueError(
highlighted_error('version is not allowed, please type '
'"mim download -h" to show the correct way.'))
if not is_installed(package):
raise RuntimeError(
highlighted_error(f'{package} is not installed. Please install it '
'first.'))

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@1bb17cd). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head eb35cce differs from pull request most recent head 74e8203. Consider uploading reports for the commit 74e8203 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #49   +/-   ##
=======================================
  Coverage        ?   63.81%           
=======================================
  Files           ?       21           
  Lines           ?     1564           
  Branches        ?      347           
=======================================
  Hits            ?      998           
  Misses          ?      431           
  Partials        ?      135           
Flag Coverage Δ
unittests 63.81% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bb17cd...74e8203. Read the comment docs.

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.

download function should not require local model_zoo.yml file
3 participants