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.

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

flatten Wandb hyperparameters dict #2459

Merged
merged 3 commits into from
Jul 8, 2020
Merged

Conversation

anthonytec2
Copy link
Contributor

What does this PR do?

Flattens Wandb Params

Fixes #2458

Before submitting

Did you have fun?

Make sure you had fun coding 🙃

@mergify mergify bot requested a review from a team July 2, 2020 01:52
@Borda Borda added the bug Something isn't working label Jul 2, 2020
@awaelchli awaelchli changed the title Wandb logging fix flatten Wandb hyperparameters dict Jul 6, 2020
Copy link
Contributor

@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.

Let's also add a changelog entry

@mergify mergify bot requested a review from a team July 6, 2020 11:13
@mergify mergify bot requested a review from a team July 6, 2020 11:19
@anthonytec2
Copy link
Contributor Author

@awaelchli I am not too familiar with the failing tests, could you explain what else I need to change?

@awaelchli
Copy link
Contributor

awaelchli commented Jul 6, 2020

ok, you need to look at this test

2020-07-06T11:17:40.1660553Z wandb = <MagicMock name='wandb' id='140405506043240'>
2020-07-06T11:17:40.1663023Z 
2020-07-06T11:17:40.1664130Z     @mock.patch('pytorch_lightning.loggers.wandb.wandb')
2020-07-06T11:17:40.1664814Z     def test_wandb_logger(wandb):
2020-07-06T11:17:40.1665473Z         """Verify that basic functionality of wandb logger works.
2020-07-06T11:17:40.1666501Z         Wandb doesn't work well with pytest so we have to mock it out here."""
2020-07-06T11:17:40.1667185Z         logger = WandbLogger(anonymous=True, offline=True)
2020-07-06T11:17:40.1667776Z     
2020-07-06T11:17:40.1668864Z         logger.log_metrics({'acc': 1.0})
2020-07-06T11:17:40.1670293Z         wandb.init().log.assert_called_once_with({'acc': 1.0})
2020-07-06T11:17:40.1670936Z     
2020-07-06T11:17:40.1671547Z         wandb.init().log.reset_mock()
2020-07-06T11:17:40.1672505Z         logger.log_metrics({'acc': 1.0}, step=3)
2020-07-06T11:17:40.1673552Z         wandb.init().log.assert_called_once_with({'global_step': 3, 'acc': 1.0})
2020-07-06T11:17:40.1674208Z     
2020-07-06T11:17:40.1675117Z         logger.log_hyperparams({'test': None})
2020-07-06T11:17:40.1676172Z >       wandb.init().config.update.assert_called_once_with({'test': None}, allow_val_change=True)
2020-07-06T11:17:40.1676728Z 
2020-07-06T11:17:40.1677311Z tests/loggers/test_wandb.py:23: 
2020-07-06T11:17:40.1677966Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2020-07-06T11:17:40.1678627Z /opt/hostedtoolcache/Python/3.6.10/x64/lib/python3.6/unittest/mock.py:825: in assert_called_once_with
2020-07-06T11:17:40.1679263Z     return self.assert_called_with(*args, **kwargs)
2020-07-06T11:17:40.1679871Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2020-07-06T11:17:40.1680370Z 
2020-07-06T11:17:40.1681382Z _mock_self = <MagicMock name='wandb.init().config.update' id='140405503563536'>
2020-07-06T11:17:40.1682393Z args = ({'test': None},), kwargs = {'allow_val_change': True}
2020-07-06T11:17:40.1683388Z expected = (({'test': None},), {'allow_val_change': True})
2020-07-06T11:17:40.1684065Z _error_message = <function NonCallableMock.assert_called_with.<locals>._error_message at 0x7fb2b46feb70>
2020-07-06T11:17:40.1685610Z actual = call({'test': 'None'}, allow_val_change=True), cause = None
2020-07-06T11:17:40.1686208Z 
2020-07-06T11:17:40.1686818Z     def assert_called_with(_mock_self, *args, **kwargs):
2020-07-06T11:17:40.1687451Z         """assert that the mock was called with the specified arguments.
2020-07-06T11:17:40.1688106Z     
2020-07-06T11:17:40.1689727Z         Raises an AssertionError if the args and keyword args passed in are
2020-07-06T11:17:40.1690285Z         different to the last call to the mock."""
2020-07-06T11:17:40.1690764Z         self = _mock_self
2020-07-06T11:17:40.1691250Z         if self.call_args is None:
2020-07-06T11:17:40.1691748Z             expected = self._format_mock_call_signature(args, kwargs)
2020-07-06T11:17:40.1692734Z             raise AssertionError('Expected call: %s\nNot called' % (expected,))
2020-07-06T11:17:40.1693254Z     
2020-07-06T11:17:40.1693715Z         def _error_message():
2020-07-06T11:17:40.1694201Z             msg = self._format_mock_failure_message(args, kwargs)
2020-07-06T11:17:40.1694674Z             return msg
2020-07-06T11:17:40.1695145Z         expected = self._call_matcher((args, kwargs))
2020-07-06T11:17:40.1695627Z         actual = self._call_matcher(self.call_args)
2020-07-06T11:17:40.1696095Z         if expected != actual:
2020-07-06T11:17:40.1696581Z             cause = expected if isinstance(expected, Exception) else None
2020-07-06T11:17:40.1697076Z >           raise AssertionError(_error_message()) from cause
2020-07-06T11:17:40.1698021Z E           AssertionError: Expected call: update({'test': None}, allow_val_change=True)
2020-07-06T11:17:40.1699531Z E           Actual call: update({'test': 'None'}, allow_val_change=True)
2020-07-06T11:17:40.1699967Z 

See the last two lines here.
The flattening and sanitizing converts the literal None to "None" (str), so you need to adjust the test.
However, now that I think about it, I see one problem:
If int, float, list, etc are converted to strings, then the hyperparamer sweep tool won't recognize these datatypes and see every value as a categorical choice. hmm, what do we do about this.
See https://docs.wandb.com/sweeps
I need to test this first

@awaelchli
Copy link
Contributor

@anthonytec2 Actually it's all fine. I looked at it and you just need to update the tests. Go to tests/loggers/test_wandb, in the function test_wandb_logger change the lines

    logger.log_hyperparams({'test': None})
    wandb.init().config.update.assert_called_once_with({'test': None}, allow_val_change=True)

to

    logger.log_hyperparams({'test': None, 'nested': {'a': 1}, 'b': [2, 3, 4]})
    wandb.init().config.update.assert_called_once_with(
        {'test': 'None', 'nested/a': 1, 'b': [2, 3, 4]},
        allow_val_change=True,
    )

As you can see, the flattening works while leaving the other attributes untouched. If you modify like this it should pass the tests.

@pep8speaks
Copy link

pep8speaks commented Jul 8, 2020

Hello @anthonytec2! Thanks for updating this PR.

Line 27:1: W293 blank line contains whitespace

Comment last updated at 2020-07-08 00:42:21 UTC

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #2459 into master will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2459   +/-   ##
======================================
- Coverage      90%     89%   -0%     
======================================
  Files          69      69           
  Lines        5669    5670    +1     
======================================
- Hits         5077    5060   -17     
- Misses        592     610   +18     

@awaelchli awaelchli requested a review from Borda July 8, 2020 02:07
@Borda Borda merged commit 899cd74 into Lightning-AI:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wandb Flatten Dict
5 participants