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.

Sign up for GitHub

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

Jump to bottom

Replace meta_tags.csv with hparams.yaml #1271

Merged
merged 50 commits into from
May 13, 2020
Merged

Replace meta_tags.csv with hparams.yaml #1271

merged 50 commits into from
May 13, 2020

Conversation

S-aiueo32
Copy link
Contributor

@S-aiueo32 S-aiueo32 commented Mar 28, 2020 •
edited
Loading

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1212 .

S-aiueo32 and others added 13 commits March 14, 2020 02:26
@S-aiueo32
Add support for hierarchical dict
af9555a
@S-aiueo32
Support nested Namespace
29ae37c
@S-aiueo32
Resolving conflict with the current master
fc7fe7c
@S-aiueo32
Add docstring
8ced285
@S-aiueo32
Migrate hparam flattening to each logger
c2e00a4
@S-aiueo32
Modify URLs in CHANGELOG
117acd7
@S-aiueo32
typo
e128430
@S-aiueo32 @Borda
Simplify the conditional branch about Namespace
5497d10 
Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>
@S-aiueo32 @Borda
Update CHANGELOG.md
891c600 
Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>
@S-aiueo32
added examples section to docstring
d54c597
@S-aiueo32
Merge branch 'feature/support-hierarchical-dict' of https://github.co…
52fe621 
…m/S-aiueo32/pytorch-lightning into feature/support-hierarchical-dict
@S-aiueo32
renamed _dict -> input_dict
377b7c4
@S-aiueo32
Merge branch 'master' of https://github.com/PyTorchLightning/pytorch-…
942895f 
…lightning
@pep8speaks
Copy link

pep8speaks commented Mar 28, 2020 •
edited
Loading

Hello @S-aiueo32! Thanks for updating this PR.

Line 20:111: E501 line too long (117 > 110 characters)
Line 1575:111: E501 line too long (117 > 110 characters)

Line 15:111: E501 line too long (118 > 110 characters)

Comment last updated at 2020-05-13 11:10:28 UTC

awaelchli
awaelchli reviewed Mar 28, 2020
View reviewed changes
Copy link
Member

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

cool. maybe a test for saving to and loading from a yaml file would be good.

pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/tensorboard.py Show resolved Hide resolved
@Borda Borda added the feature Is an improvement or enhancement label Mar 29, 2020
Borda
Borda reviewed Mar 29, 2020
View reviewed changes
requirements.txt Outdated
@@ -5,4 +5,4 @@ numpy>=1.16.4
torch>=1.1
tensorboard>=1.14
future>=0.17.1 # required for builtins in setup.py

pyyaml>=5.3.1
Copy link
Member

Choose a reason for hiding this comment

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

is it really the minimal version we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the current latest version

Copy link
Member

Choose a reason for hiding this comment

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

Could we put there like 3.x or lover since we are using quite general features?

Copy link
Member

Choose a reason for hiding this comment

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

@S-aiueo32 Don't forget to also add it to the environment.yml file

@mergify
Copy link
Contributor

mergify bot commented Mar 30, 2020

This pull request is now in conflict... :(

@mergify mergify bot requested a review from a team March 30, 2020 22:33
@Borda Borda added this to the 0.7.2 milestone Apr 2, 2020
@Borda
Copy link
Member

Borda commented Apr 2, 2020

@S-aiueo32 how is it going? it would be great to have it in next week release 0.7.2

@S-aiueo32
Copy link
Contributor Author

@Borda I'm sorry, but I'm too busy for about two weeks this week to be able to respond. I recognize that the major changes are complete and all that remains is testing and minor revisions. If you don't mind, could you let the core contributor take over the work?

@Borda Borda self-assigned this Apr 2, 2020
@williamFalcon
Copy link
Contributor

@Borda mind taking over this one?

@williamFalcon williamFalcon modified the milestones: 0.7.2, 0.7.3 Apr 3, 2020
@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2020

This pull request is now in conflict... :(

@Borda
Copy link
Member

Borda commented Apr 8, 2020

changelog need to be rebased on new release #1419

@Borda
Copy link
Member

Borda commented Apr 11, 2020

@S-aiueo32 may you pls allow editing your PR, I have prepared to rebase, but don't have permission to push it... 🤖

@S-aiueo32
Copy link
Contributor Author

S-aiueo32 commented Apr 12, 2020 •
edited by Borda
Loading

@Borda confirmed?

@S-aiueo32 S-aiueo32 force-pushed the feature/replace-mata-tags-with-yaml branch from bc64951 to 5d27e34 Compare May 13, 2020 11:05
@S-aiueo32
Copy link
Contributor Author

@Borda @awaelchli @williamFalcon All checks pass, thanks!

@Borda
Copy link
Member

Borda commented May 13, 2020

it looks good to me, I have pinged @PyTorchLightning/core-contributors to get this done

justusschock
justusschock approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

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

LGTM

@Borda Borda requested a review from jeremyjordan May 13, 2020 13:04
@Borda Borda merged commit 22d7d03 into Lightning-AI:master May 13, 2020
@Borda
Copy link
Member

Borda commented May 13, 2020

@S-aiueo32 Great work! and thx for your patience 🦝

jeremyjordan
jeremyjordan approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@jeremyjordan jeremyjordan left a comment

Choose a reason for hiding this comment

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

shall we include a test for tests/test_deprecated.py?

@Borda
Copy link
Member

Borda commented May 13, 2020

shall we include a test for tests/test_deprecated.py?

completely forgot... @S-aiueo32 mind make followup or with deprecation tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

@justusschock justusschock justusschock approved these changes

@jeremyjordan jeremyjordan jeremyjordan approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli awaelchli is a code owner

Assignees

@Borda Borda

Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.7.x
Development

Successfully merging this pull request may close these issues.

Add support for loading flattened meta_tags.csv
8 participants
@S-aiueo32 @pep8speaks @Borda @williamFalcon @awaelchli @monney @justusschock @jeremyjordan