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

Fix checkpoint loader to use GFile #938

Merged
merged 2 commits into from
Mar 23, 2020
Merged

Fix checkpoint loader to use GFile #938

merged 2 commits into from
Mar 23, 2020
+1 −1

Conversation

kchygoe
Copy link
Member

@kchygoe kchygoe commented Mar 19, 2020

What this patch does to fix the issue.

When OUTPUT_DIR=gs://xxxx/xxx,
Training cannot be kept running.
This is critical for gcs/s3 users

@blueoil-butler blueoil-butler bot added the CI: auto-run Run CI automatically label Mar 19, 2020
@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

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

@kchygoe kchygoe requested a review from tfujiwar March 19, 2020 11:58
tfujiwar
tfujiwar approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@tfujiwar tfujiwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability Approval (reverse-shadowing)
Nice bug fixing!!

@kchygoe kchygoe requested review from iizukak and tsawada March 20, 2020 07:56
@kchygoe kchygoe self-assigned this Mar 20, 2020
iizukak
iizukak approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@iizukak iizukak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OA

tsawada
tsawada reviewed Mar 23, 2020
View reviewed changes
blueoil/cmd/train.py Outdated
with open(checkpoint) as stream:
data = yaml.load(stream)
with tf.io.gfile.GFile(checkpoint) as stream:
data = yaml.load(stream, Loader=yaml.FullLoader)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this loader change related to GFile change?

It seems there are a few other use of yaml.load within blueoil thus maybe we should fix all in another diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is not directly related.

I just noticed that load method has been changed to specify Loader .
Let me dedicate them as you commented.
https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@kchygoe kchygoe requested a review from tsawada March 23, 2020 06:42
tsawada
tsawada approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@tsawada tsawada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability Approval

@kchygoe
Copy link
Member Author

kchygoe commented Mar 23, 2020

/ready

@bo-mergebot
Copy link
Contributor

bo-mergebot bot commented Mar 23, 2020

⏳Merge job is queued...

@kchygoe kchygoe mentioned this pull request Mar 23, 2020
Closed
@bo-mergebot bo-mergebot bot merged commit 969633c into blue-oil:master Mar 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iizukak iizukak iizukak approved these changes

@tsawada tsawada tsawada approved these changes

@tfujiwar tfujiwar tfujiwar approved these changes

Assignees

@kchygoe kchygoe

Labels
CI: auto-run Run CI automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kchygoe @iizukak @tsawada @tfujiwar