blueoil icon indicating copy to clipboard operation
blueoil copied to clipboard

fix for loading class names when using convert (and class names are property)

Open joelN123 opened this issue 4 years ago • 6 comments

This is to fix the bug described in issue https://github.com/blue-oil/blueoil/issues/1076

The error occurs because when running convert, the _saved_config_file_path function will retrieve the config.py file, and it will try to access classes property of the dataset class. However, the dataset hasn't been instantiated, so it can't access the class names.

One way around it is to create a dummy dataset, and use the classes property of that. But it adds a few lines of code, and creating a dummy dataset only for this purpose is not ideal.

Another way (the way in this PR), is to prefer to load config.yaml, rather than config.py. The config.yaml has the full class names directly listed because of this existing code, which is run when doing training.

The implementation is very simple, just swapping config.py and config.yaml in one line changes the order of preference for which one to use (only one is used, not both).

p.s. "'config.yaml' is duplication of python config file as yaml." so it seems fine to prefer config.yaml over config.py when running the conversion.

I tested it by doing training and converting a model similar to lm_resnet.py, for Ilsvrc2012 dataset. By doing the change shown in this PR, it could convert.

p.s. the original issue mentions DarknetQuantize. However, after doing the fix in this PR, I still get an error when converting it AssertionError: Pooling operator pool_1_MaxPool does not match the height: 224 vs 112. I am pretty sure it is a different error, not related to the issue of loading class names.

joelN123 avatar Jun 16 '20 13:06 joelN123

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 16 '20 13:06 CLAassistant

This PR needs Approvals as follows.

  • Ownership Approval for / from iizukak, tkng, ruimashita
  • Readability Approval for Python from tkng, tsawada, tfujiwar

Please choose reviewers and requet reviews!

Click to see how to approve each reviews

You can approve this PR by triggered comments as follows.

  • Approve all reviews requested to you (readability and ownership) and LGTM review Approval, LGTM

  • Approve all ownership reviews Ownership Approval or OA

  • Approve all readability reviews Readability Approval or RA

  • Approve specified review targets

    • Example of Ownership Reviewer of /: Ownership Approval for / or OA for /
    • Example of Readability Reviewer of Python: Readability Approval for Python or RA for Python
  • Approve LGTM review LGTM

See all trigger comments

Please replace [Target] to review target

  • Ownership Approval
    • Ownership Approval for [Target]
    • OA for [Target]
    • Ownership Approval
    • OA
    • Approval
  • Readability Approval
    • Readability Approval for [Target]
    • RA for [Target]
    • [Target] Readability Approval
    • [Target] RA
    • Readability Approval
    • RA
    • Approval
  • LGTM
    • LGTM
    • lgtm

bo-code-review-bot[bot] avatar Jun 16 '20 13:06 bo-code-review-bot[bot]

@joelN123 Thank you for the contribution. This PR cause some errors on CI. Can you check?

iizukak avatar Jun 22 '20 03:06 iizukak

thank you! Yes, sorry, I didn't write an update here yet. Essentially, a different error comes up after making this change. The error is basically not related to this one, so I'm writing a separate PR for it. Then after that PR is merged, I expect this one will pass the checks without needing any changes.

joelN123 avatar Jun 22 '20 05:06 joelN123

@joelN123 OK Thanks.

iizukak avatar Jun 22 '20 05:06 iizukak

The separate PR is over here https://github.com/blue-oil/blueoil/pull/1110 "fix loading of dataset class from yaml config" (still waiting for auto-tests to finish). I expect it will solve the checks here because the config.yaml will be able to load.

joelN123 avatar Jun 22 '20 09:06 joelN123