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

Clean-up after logger connector redesign 1/2 #7909

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Jun 9, 2021

What does this PR do?

Smile and wave, boys 👋

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • [n/a] Did you make sure to update the documentation with your changes? (if necessary)
  • [n/a] Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Copy link
Contributor

@ananthsub ananthsub left a comment

Choose a reason for hiding this comment

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

Amazing work @carmocca !

@carmocca carmocca added the logging Related to the `LoggerConnector` and `log()` label Jun 9, 2021
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

just an n00bQ, as all these were publically exposed API, shall we do a deprecation cycle?

@awaelchli awaelchli enabled auto-merge (squash) June 9, 2021 23:32
@carmocca
Copy link
Contributor Author

carmocca commented Jun 9, 2021

just an n00bQ, as all these were publically exposed API, shall we do a deprecation cycle?

It's too late to ask that since all this code is currently unused in master.

And even if we wanted to, I'm calling this a redesign instead of a refactor because it has changed almost entirely. If we wanted to fully deprecate everything it would have taken ages.

Note that all the important user-facing attributes have been kept, namely the access to trainer.{callback,progress_bar,logged}_metrics. Everything else inside the logger connector and result files was supposed to be internal.

In any case, if you have concerns about specific pieces of the changes, we can discuss it and see if we can keep BC for them.

@Borda
Copy link
Member

Borda commented Jun 9, 2021

If we wanted to fully deprecate everything it would have taken ages.

I do not think we would need deprecate all (parallel to past accelerators), just think if some of these classes could be used by user independently and ten deprecate them... no need for purely internal use...

In any case, if you have concerns about specific pieces of the changes, we can discuss it and see if we can keep BC for them.

that was my goal, if there are some places that could be used by the user specifically outside PL loop

@awaelchli awaelchli merged commit df81239 into master Jun 10, 2021
@awaelchli awaelchli deleted the refactor/clean-after-logger-connector-redesign branch June 10, 2021 05:21
Result.attach_batch_size(batch_size, self)

@staticmethod
def extract_batch_size(batch):
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a few downstream projects which take a dependency on this utility function.

In the short-term, we're replicating the logic within those projects. Is this the right long-term approach? Or are there plans to maintain and expose these utility functions? The new ResultCollection has a similar function (https://github.com/PyTorchLightning/pytorch-lightning/pull/7882/files#diff-bf4a466b765dd5c4128aca411d2999df7d8b6283723ea721be94713dac190112R444), but this is not static and requires a Result instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it's better for you to replicate the function since it wasn't originally meant to be used for users.
But it's completely fair that we keep BC for it since it wasn't previously protected.

Do you want to open a PR so it becomes static again? Could also include some unit tests for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging Related to the `LoggerConnector` and `log()` refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants