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

Rename profiler directory to profilers #12308

Merged

Conversation

Pragyanstha
Copy link

@Pragyanstha Pragyanstha commented Mar 12, 2022

[rebase and merge]

What does this PR do?

Closes #11972.

  • Consists of two commits:
    • Commit 1:
      • rename profiler/ directory to profilers/.
      • update import statements all over the repository.
    • Commit 2:
      • reroute profiler/ to profilers/ with the deprecation warning.
      • put back items that had already been deprecated in the previous PRs.

Does your PR introduce any breaking changes? If yes, please list them.

No, it just deprecates profiler/* in favour of profilers/*.

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?
  • 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 list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • 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

Did you have fun?

maybe

@Pragyanstha Pragyanstha marked this pull request as ready for review March 12, 2022 08:34
@rohitgr7 rohitgr7 changed the title Refactor/#11972 rename profiler Rename profiler directory to profilers Mar 16, 2022
rohitgr7
rohitgr7 previously approved these changes Mar 16, 2022
Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

LGTM! mind add a changelog entry?

pytorch_lightning/profilers/__init__.py Outdated Show resolved Hide resolved
@kaushikb11
Copy link
Contributor

A second PR would be created for adding back the previous API that redirects the call into the new API,
with a deprecation warning.

I think you should add it in this PR

@rohitgr7
Copy link
Contributor

A second PR would be created for adding back the previous API that redirects the call into the new API,
with a deprecation warning.

I think you should add it in this PR

it will create a lot of changes in a single one since git file-renames won't be valid then.

@carmocca carmocca added this to the 1.7 milestone Mar 21, 2022
carmocca
carmocca previously approved these changes Mar 21, 2022
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

LGTM but this should go after the release to make sure we have time for the deprecation too.

.github/CODEOWNERS Outdated Show resolved Hide resolved
tchaton
tchaton previously approved these changes Mar 22, 2022
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

@daniellepintz
Copy link
Contributor

@Pragyanstha mind rebasing this PR?

@rohitgr7 rohitgr7 force-pushed the refactor/#11972_rename_profiler branch from 3b87abe to cb256c1 Compare March 24, 2022 14:54
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Mar 24, 2022
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.

As is, this PR is a breaking change for those on master, which we should avoid.

I would strongly recommend doing both of these in the same PR to avoid any breaking change:

  • Copy the existing code over to a profilers/ directory
  • In the existing profiler/ directory, import the components from the new profilers/ directory and include the relevant deprecation warnings

@mergify mergify bot removed the ready PRs ready to be merged label Mar 24, 2022
@rohitgr7
Copy link
Contributor

rohitgr7 commented Mar 25, 2022

As is, this PR is a breaking change for those on master, which we should avoid.

I would strongly recommend doing both of these in the same PR to avoid any breaking change:

* Copy the existing code over to a profilers/ directory

* In the existing profiler/ directory, import the components from the new profilers/ directory and include the relevant deprecation warnings

wouldn't that create a huge git diff for reviewers? I think we did the same for training type plugin -> strategy. This is targeted after v1.6, so we can quickly create another one once this is merged.

awaelchli added a commit to Pragyanstha/pytorch-lightning that referenced this pull request Jun 21, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
@awaelchli awaelchli force-pushed the refactor/#11972_rename_profiler branch from ed8001d to 49a4ed7 Compare June 21, 2022 17:13
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Jun 21, 2022
awaelchli added a commit to Pragyanstha/pytorch-lightning that referenced this pull request Jun 21, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
@awaelchli awaelchli force-pushed the refactor/#11972_rename_profiler branch from 230f6ce to e479409 Compare June 21, 2022 17:26
@awaelchli awaelchli requested a review from carmocca June 21, 2022 18:18
CHANGELOG.md Show resolved Hide resolved
awaelchli added a commit that referenced this pull request Jun 22, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
awaelchli added a commit to Pragyanstha/pytorch-lightning that referenced this pull request Jun 22, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
awaelchli added a commit to Pragyanstha/pytorch-lightning that referenced this pull request Jun 22, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
@awaelchli awaelchli force-pushed the refactor/#11972_rename_profiler branch from e479409 to ea56327 Compare June 22, 2022 14:57
@awaelchli awaelchli requested a review from carmocca June 22, 2022 14:58
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
carmocca added a commit to Pragyanstha/pytorch-lightning that referenced this pull request Jun 22, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
@carmocca carmocca force-pushed the refactor/#11972_rename_profiler branch from ea56327 to 4d32e61 Compare June 22, 2022 15:56
@carmocca
Copy link
Contributor

Included a change in the last commit that should fix the cli failures

Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
@carmocca carmocca force-pushed the refactor/#11972_rename_profiler branch from 4d32e61 to 7090ba9 Compare June 22, 2022 16:45
@awaelchli awaelchli disabled auto-merge June 22, 2022 18:22
@lexierule lexierule merged commit 511f1a6 into Lightning-AI:master Jun 23, 2022
lexierule pushed a commit that referenced this pull request Jun 23, 2022
Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR is from the community deprecation Includes a deprecation profiler ready PRs ready to be merged refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename profiler/ directory to profilers/