-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 paddle hub #31873
Add paddle hub #31873
Conversation
Thanks for your contribution! |
python/paddle/hapi/hub.py
Outdated
```python | ||
import paddle | ||
|
||
paddle.hub.help('lyuwenyu/PaddleClas:hub_L', source='github', force_reload=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using official repo will be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. It's testing code, beacuse PaddleCls dont have hub.conf by now.
Need a simple unittest |
Load model | ||
|
||
Args: | ||
repo_dir(str): github or local path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as offline discussion, also can add gitee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will push it after testing offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
python/paddle/hapi/hub.py
Outdated
repo_dir, force_reload, True, source=source) | ||
|
||
sys.path.insert(0, repo_dir) | ||
hub_module = __import__(MODULE_HUBCONF.split('.')[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that sys.modules['hubconf']
will be used by default on the second load, which will causes unexpected behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I should pop
it after __import__
module to keep sys.module
clean
sys.modules.pop('hubconf')
ps. using importlib seems beeter, but it‘s incompatible in py2 and py3 😢
python/paddle/hapi/hub.py
Outdated
# Setup hub_dir to save downloaded files | ||
hub_dir = HUB_DIR | ||
if not os.path.exists(hub_dir): | ||
os.makedirs(hub_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that multi-process calls may cause the problem of repeated directory creation and raise an exception.
suggest:
os.makedirs(hub_dir, exist_ok=True)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy~
python/paddle/hapi/hub.py
Outdated
func = getattr(m, name, None) | ||
|
||
if func is None or not callable(func): | ||
raise RuntimeError('Canot find callable {} in hubconf'.format(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canot
-> Cannot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy~
python/paddle/hapi/hub.py
Outdated
Args: | ||
repo_dir(str): github or local path | ||
github path (str): a str with format "repo_owner/repo_name[:tag_name]" with an optional | ||
tag/branch. The default branch is `master` if not specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default是main还是master
MASTER_BRANCH
定义是main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
应该是main
, 我修改下
python/paddle/hapi/hub.py
Outdated
```python | ||
import paddle | ||
|
||
paddle.hub.help('lyuwenyu/paddlehub_demo:main', source='github', force_reload=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
示例的api不对
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it ~😢
python/paddle/hapi/hub.py
Outdated
Args: | ||
repo_dir(str): github or local path | ||
github path (str): a str with format "repo_owner/repo_name[:tag_name]" with an optional | ||
tag/branch. The default branch is `master` if not specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同上
python/paddle/hapi/hub.py
Outdated
Args: | ||
repo_dir(str): github or local path | ||
github path (str): a str with format "repo_owner/repo_name[:tag_name]" with an optional | ||
tag/branch. The default branch is `master` if not specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同上
python/paddle/hapi/hub.py
Outdated
github path (str): a str with format "repo_owner/repo_name[:tag_name]" with an optional | ||
tag/branch. The default branch is `master` if not specified. | ||
local path (str): local repo path | ||
mdoel (str): model name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
np.testing.assert_equal(out.shape, [1, 8, 50, 50]) | ||
|
||
model = hub.load( | ||
self.github_repo, model='MM', source='github', force_reload=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是否能检查下load之后的结果是否正确?
这几个参数是否发挥作用,这个用例并不能很好的测试出来。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 已修改, 增加固定输入和pretrained参数并测试输出的正确性
这几个参数是否发挥作用
,是的 一个用例不能很好的反馈出来 但是结合上下文多个tests按顺序是隐式的 1. hub.loadsource
local和remote模型使用的地址不一致,2. 首次使用remote时候本地是没有缓存的 当为False时候不会download数据 测试能过说明前边为True时候起作用了,3. 不符合要求的参数 在异常里测试的
python/paddle/tests/test_hapi_hub.py
Outdated
docs = hub.help( | ||
self.github_repo, model='MM', source='github', force_reload=False) | ||
|
||
print(docs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
检查下docs的值是否正确?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯可以
python/paddle/tests/test_hapi_hub.py
Outdated
source='github', | ||
force_reload=False, ) | ||
|
||
print(models) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
检查下list的执行结果是否符合预期?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯可以
ae0bb17
python/paddle/hapi/hub.py
Outdated
@@ -19,7 +19,7 @@ | |||
import zipfile | |||
from paddle.utils.download import get_path_from_url | |||
|
|||
MASTER_BRANCH = 'main' | |||
MAIN_BRANCH = 'main' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
变量名称是不是应该叫DEFAULT_BRANCH
另外,是不是github的时候default是main, gitee的时候default是master,比较合理。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR types
New features
PR changes
Others
Describe