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

Added PhysicsTools/PyTorch and new category for ML tools #2121

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

valsdav
Copy link
Contributor

@valsdav valsdav commented Dec 1, 2023

Adding category to new package introduced in PR cms-sw/cmssw#43475

(Please let me know if this is the correct procedure)

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2023

A new Pull Request was created by @valsdav (Davide Valsecchi) for branch master.

@aandvalenzuela, @iarspider, @smuzaffar, @cmsbuild can you please review it and eventually sign? Thanks.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.
cms-bot commands are listed here

@makortel
Copy link
Contributor

makortel commented Dec 1, 2023

@cms-sw/reconstruction-l2

@makortel
Copy link
Contributor

makortel commented Dec 1, 2023

I've started to wonder if reconstruction continues to be the best place for ML (inference) tools?

Or would it be feasible, or make sense, for ML group to take the maintenance ownership of the ML tools? (I understand this question is bigger than the immediate need, and probably should be discussed elsewhere)

@makortel
Copy link
Contributor

makortel commented Dec 1, 2023

(Please let me know if this is the correct procedure)

It is :)

@smuzaffar
Copy link
Contributor

@cms-sw/reconstruction-l2 what do you think?

@valsdav
Copy link
Contributor Author

valsdav commented Dec 4, 2023

I've started to wonder if reconstruction continues to be the best place for ML (inference) tools?

Or would it be feasible, or make sense, for ML group to take the maintenance ownership of the ML tools? (I understand this question is bigger than the immediate need, and probably should be discussed elsewhere)

Hi @makortel we can discuss it further of course, but from the ML group side we are willing to step in and take the responsibility for the ML tools packages (as from the mandate of the ML production subgroup).

@tvami
Copy link
Contributor

tvami commented Dec 4, 2023

We could also have it under the analysis signature. Not a strong push, just an idea since a few of this stuff is already under that signiture.

@makortel
Copy link
Contributor

makortel commented Dec 4, 2023

I've started to wonder if reconstruction continues to be the best place for ML (inference) tools?
Or would it be feasible, or make sense, for ML group to take the maintenance ownership of the ML tools? (I understand this question is bigger than the immediate need, and probably should be discussed elsewhere)

Hi @makortel we can discuss it further of course, but from the ML group side we are willing to step in and take the responsibility for the ML tools packages (as from the mandate of the ML production subgroup).

Hi @valsdav. I have absolutely no objections if the matter can be settled already within this issue :) And thank you for being willing to step up. How about we'd then add ml signatory category and GitHub team, and move

PhysicsTools/ONNXRuntime
PhysicsTools/TensorFlow
PhysicsTools/PyTorch

from reconstruction to ml? (note that this change would also imply that someone from the ML team would have to attend the ORP meeting on Tuesdays; in case you didn't already)

The PhysicsTools/MXNet is probably simplest to keep in reconstruction, given that the package has been removed (but needs to be kept somewhere)?

I'm not really sure if something should be done with

PhysicsTools/MVAComputer
PhysicsTools/MVATrainer

that sound ML related, and are currently under analysis.

I'd certainly wait for @cms-sw/reconstruction-l2 to comment before doing anything.

@makortel
Copy link
Contributor

makortel commented Dec 4, 2023

We could also have it under the analysis signature. Not a strong push, just an idea since a few of this stuff is already under that signiture.

@tvami While some ML stuff is currently under analysis, that doesn't seem a very good fit to me because of e.g.

  • ML is used in many areas beyond analysis
    • Would analysis e.g. debug ML-related problems in reconstruction, simulation, L1T, etc. code?
  • analysis was unattended for a long time (I'm glad the situation changed recently! but given the past I see risks)
  • We have a specific ML group :)

@tvami
Copy link
Contributor

tvami commented Dec 4, 2023

hi Matti, yes, what say all makes sense, I agree

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 4, 2023

Hi,
I believe It makes totally sense, I Will check with @mandrenguyen tomorrow and come back here.
Thanks!

@mandrenguyen
Copy link
Contributor

ok for me

@valsdav
Copy link
Contributor Author

valsdav commented Dec 5, 2023

I've started to wonder if reconstruction continues to be the best place for ML (inference) tools?
Or would it be feasible, or make sense, for ML group to take the maintenance ownership of the ML tools? (I understand this question is bigger than the immediate need, and probably should be discussed elsewhere)

Hi @makortel we can discuss it further of course, but from the ML group side we are willing to step in and take the responsibility for the ML tools packages (as from the mandate of the ML production subgroup).

Hi @valsdav. I have absolutely no objections if the matter can be settled already within this issue :) And thank you for being willing to step up. How about we'd then add ml signatory category and GitHub team, and move

PhysicsTools/ONNXRuntime
PhysicsTools/TensorFlow
PhysicsTools/PyTorch

from reconstruction to ml? (note that this change would also imply that someone from the ML team would have to attend the ORP meeting on Tuesdays; in case you didn't already)

The PhysicsTools/MXNet is probably simplest to keep in reconstruction, given that the package has been removed (but needs to be kept somewhere)?

I'm not really sure if something should be done with

PhysicsTools/MVAComputer
PhysicsTools/MVATrainer

that sound ML related, and are currently under analysis.

I'd certainly wait for @cms-sw/reconstruction-l2 to comment before doing anything.

Hi @makortel I agree with you about the analysis category. It won't be a problem for us to follow the ORP meeting.
I will check what's inside MVATrainer and MVAComputer

@smuzaffar
Copy link
Contributor

So if I understand it correctly

PhysicsTools/ONNXRuntime
PhysicsTools/TensorFlow
PhysicsTools/PyTorch

should move to ml category. PhysicsTools/MVAComputer and PhysicsTools/MVATrainer can also move under ml.

If no objections from others then @valsdav please update categories_map.py and also update CMSSW_L2 in https://github.com/cms-sw/cms-bot/blob/master/categories.py to responsible for new ml category

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 7, 2023

Pull request #2121 was updated.

@valsdav
Copy link
Contributor Author

valsdav commented Dec 7, 2023

FYI @wpmccormack

@valsdav valsdav changed the title Added PhysicsTools/PyTorch Added PhysicsTools/PyTorch and creating new category for ML tools Dec 7, 2023
@valsdav valsdav changed the title Added PhysicsTools/PyTorch and creating new category for ML tools Added PhysicsTools/PyTorch and new category for ML tools Dec 7, 2023
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 7, 2023

Pull request #2121 was updated.

"PhysicsTools/ONNXRuntime",
"PhysicsTools/MVAComputer",
"PhysicsTools/MVATrainer",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

@valsdav , this block should be added in CMSSW_CATEGORIES instead of CMSSW_LABELS. Please move it to

"operations": [

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 8, 2023

Pull request #2121 was updated.

@smuzaffar smuzaffar merged commit 1970e3c into cms-sw:master Dec 8, 2023
3 of 4 checks passed
@makortel
Copy link
Contributor

makortel commented Dec 8, 2023

@smuzaffar Should this PR lead to the creation of the cms-sw/ml-l2 GitHub team? Or does that need some further setup?

@smuzaffar
Copy link
Contributor

@makortel , yes cms-sw/ml-l2 team will be created automatically. The Jenkins job which creates these teams runs once a day . I will force run it now to get this team setup

@smuzaffar
Copy link
Contributor

cms-sw/ml-l2 has been setup and bot have sent the invitations to @valsdav and @wpmccormack to join this team

@makortel
Copy link
Contributor

makortel commented Dec 8, 2023

Ok, thanks (so I was just impatient)

@valsdav
Copy link
Contributor Author

valsdav commented Dec 8, 2023

Done, thanks!!

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

Successfully merging this pull request may close these issues.

7 participants