Skip to content

[Feedback] Warn Developers Regarding Major Code Changes #5710

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

Open
janeyx99 opened this issue Mar 30, 2022 · 6 comments
Open

[Feedback] Warn Developers Regarding Major Code Changes #5710

janeyx99 opened this issue Mar 30, 2022 · 6 comments

Comments

@janeyx99
Copy link
Contributor

🚀 The feature

Flag parts of the repo that are undergoing major changes (breaking changes between two releases) + have a bot post on PRs that touch these high traffic files.

Motivation, pitch

TL;DR: torchvision moves pretty fast and external developers want to know if they are developing on something that may change very soon.

Imagine you are an interested developer and you would love to contribute to torchvision. You scan through the issues and you see an interesting one you can help tackle! You follow some directions, get right into the task, and a few days later, you have a PR. You iterate based on CI results and you're pretty proud of your feature.

You get a reviewer to look through and hopefully approve your awesome change. BUT that's not what happens. Instead, the reviewer lets you know that the code you're touching is actually migrating to some other structure and the work you did on your PR has become irrelevant. What a downer. You wish you knew this earlier, before you spent so much time on the PR.

The current system can be quite discouraging for an external developer. To promote contributions, torchvision should notify developers of major code changes in some way. One way is to flag those paths that are likely changing soon and having a bot post on PRs touching those paths with a warning message. This way, developers have more clarity on what they should work on.

Alternatives

Other alternatives are for torchvision to update existing issues/feature requests with up-to-date content so that developers can fix an issue knowing that their work would not be in vain.

Additional context

This feedback was gathered from talking to current external contributors.

@datumbox
Copy link
Contributor

datumbox commented Mar 31, 2022

@janeyx99 Thanks for the feedback!

Finding more ways to warn developers for upcoming major code changes sounds a reasonable proposal.

Note that this is something we currently already do to a degree. For example we notify users via blogposts several months prior actually landing the feature on main branch, include notes about upcoming API changes on our previous release notes and we actively seek the feedback of the community both on Github and on other channels during decision making.

One way is to flag those paths that are likely changing soon and having a bot post on PRs touching those paths with a warning message.

Is this a solution that PyTorch core currently uses? Could you provide an example of how this is configured?

You get a reviewer to look through and hopefully approve your awesome change. BUT that's not what happens.

Could you provide a few instances when this occurred? It can help us identify issues in our process.

@janeyx99
Copy link
Contributor Author

janeyx99 commented Apr 1, 2022

Note that this is something we currently already do to a degree.

Yes, this is something I think torchvision does better than core torch, though torchvision also moves a lot faster on the whole.

One way is to flag those paths that are likely changing soon and having a bot post on PRs touching those paths with a warning message.

Is this a solution that PyTorch core currently uses? Could you provide an example of how this is configured?

Core does not have a solution like this just yet--but core also could do better in this regard as well. This suggestion was derived from talking with @oke-aditya and @frgfm.

Could you provide a few instances when this occurred? It can help us identify issues in our process.

I'll get back to you if I gather any specific examples--feel free to deprioritize as this doesn't seem too great a concern for vision (given your many other avenues of communication).

@datumbox
Copy link
Contributor

datumbox commented Apr 1, 2022

Sounds great, if core adopts such a solution I would be happy to try it out here.

No rush about the specific examples. I'm still interested to get them as a learning exercise and to improve. I'm sure we can definitely do better :)

@frgfm
Copy link
Contributor

frgfm commented May 25, 2022

Alright, sorry about my very late feedback on this suggestion 😅

There are three aspects that would be key to a good contribution feature:

  • defining the type of flags that are relevant (breaking change? upcoming revamp? etc?)
  • being able to mark code sections automatically (or requiring minimal manual input)
  • using the previous information, have the CI check the code sections involved in the PR, and warn the author (either linked the PR that made those changes or find another way to do it)

I'd be happy to gather some feedback on that proposal (either here or on core 🤷 )

@pmeier
Copy link
Collaborator

pmeier commented May 26, 2022

using the previous information, have the CI check the code sections involved in the PR, and warn the author (either linked the PR that made those changes or find another way to do it)

@janeyx99 That should be doable with a similar approach as core uses for the bot. Basically, we could have configuration file somewhere that specifies the files and the flag (upcoming revamp / deprecation / ...). Now we can write a small workflow that runs every time someone pushes to a PR and cross ref the changed files in the PR with the configuration and post a message if there is overlap. Of course we would need to disable this for a group of core maintainers to avoid spam.

@frgfm
Copy link
Contributor

frgfm commented Jun 8, 2022

Yes, I can see how to do a small PR if you think it's relevant:

  • put a yaml file in .github to specify the people to ignore (core maintainers), the files and flags
  • put a python script that will be run at each PR to assess whether a message should be sent with the corresponding information
  • adding a small workflow with a PR event trigger (question: should it run only when the PR opens or when updated as well? I figured the second, but that might generate floading)

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants