Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joelN123
Copy link
Contributor

This is to fix the bug described in issue #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.

@blueoil-butler blueoil-butler bot added the CI: auto-run Run CI automatically label Jun 16, 2020
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2020

CLA assistant check
All committers have signed the CLA.

@bo-code-review-bot
Copy link

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

@iizukak
Copy link
Member

iizukak commented Jun 22, 2020

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

@joelN123
Copy link
Contributor Author

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.

@iizukak
Copy link
Member

iizukak commented Jun 22, 2020

@joelN123 OK Thanks.

@joelN123
Copy link
Contributor Author

The separate PR is over here #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 joelN123 self-assigned this Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI: auto-run Run CI automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants