search

ganga-devs / ganga

Ganga is an easy-to-use frontend for job definition and management
GNU General Public License v3.0
100 stars 159 forks source link

runMonitoring take more types addition #2390

Open saadkhi opened 3 weeks ago

saadkhi commented 3 weeks ago

The issue solved was to make runMonitoring accept different job inputs—specifically, an integer (job ID), a list of integers (job IDs), and a job object—in addition to the original job slice format. This increases flexibility in specifying jobs for monitoring.

fixed #2380

mesmith75 commented 3 weeks ago

You seem to have included your virtualenv in the commit - could you remove that?

You also seem to have touched a lot of things that are best left alone. Also I think some issues have occurred with indenting/whitespace etc. Could you try and tidy that up?

The issue only needs some pretty minimal changes so could you try and tidy this up to only include the minimal adjustments necessary?

saadkhi commented 3 weeks ago

Yes, I will make changes and update soon.

egede commented 2 weeks ago

There is something odd going on with the testing here. Not sure what as it seems unrelated to the code changes that you have made.

alexanderrichards commented 2 weeks ago

@saadkhi I think for the failure of the autopep8 test, when you created the pull request did you click the checkbox to allow upstream developers to make changes to your branch? My best guess at the moment for whats happening here is it's trying to create the autopep8 branch to pull against your branch Enh_int

egede commented 2 weeks ago

@saadkhi Take a look at the GangaCore Integration tests. There are 3 errors there that relate to the code that you modified. Please take a look first yourself and then ask for help if you can't figure out what is wrong.

Check the full details. Extract below.

FAILED ganga/GangaCore/test/GPI/Monitoring/TestMonitoring.py::TestMonitoring::test_b_EnableMonitoring - TypeError: AsyncMonitoringService.runMonitoring() got an unexpected keyword argument 'steps'
FAILED ganga/GangaCore/test/GPI/Monitoring/TestMonitoring.py::TestMonitoring::test_g_runMonitoring_withJobIDList - TypeError: AsyncMonitoringService.runMonitoring() got an unexpected keyword argument 'steps'
FAILED ganga/GangaCore/test/GPI/Monitoring/TestMonitoring.py::TestMonitoring::test_h_runMonitoring_withJobObject - TypeError: AsyncMonitoringService.runMonitoring() got an unexpected keyword argument 'steps'
saadkhi commented 2 weeks ago

@saadkhi I think for the failure of the autopep8 test, when you created the pull request did you click the checkbox to allow upstream developers to make changes to your branch? My best guess at the moment for whats happening here is it's trying to create the autopep8 branch to pull against your branch Enh_int

I'm not sure about it, let me check and fix what is going wrong with it.

saadkhi commented 2 weeks ago

@saadkhi Take a look at the GangaCore Integration tests. There are 3 errors there that relate to the code that you modified. Please take a look first yourself and then ask for help if you can't figure out what is wrong.

Check the full details. Extract below.

FAILED ganga/GangaCore/test/GPI/Monitoring/TestMonitoring.py::TestMonitoring::test_b_EnableMonitoring - TypeError: AsyncMonitoringService.runMonitoring() got an unexpected keyword argument 'steps'
FAILED ganga/GangaCore/test/GPI/Monitoring/TestMonitoring.py::TestMonitoring::test_g_runMonitoring_withJobIDList - TypeError: AsyncMonitoringService.runMonitoring() got an unexpected keyword argument 'steps'
FAILED ganga/GangaCore/test/GPI/Monitoring/TestMonitoring.py::TestMonitoring::test_h_runMonitoring_withJobObject - TypeError: AsyncMonitoringService.runMonitoring() got an unexpected keyword argument 'steps'

yes, @egede I'm working on it to figure out why these test cases get failling.

saadkhi commented 2 weeks ago

@saadkhi I think for the failure of the autopep8 test, when you created the pull request did you click the checkbox to allow upstream developers to make changes to your branch? My best guess at the moment for whats happening here is it's trying to create the autopep8 branch to pull against your branch Enh_int

I’m having trouble understanding what autopep8 is and how it applies to my pull request. Could you please explain it, or if possible, arrange a meeting sometime this week to discuss?

alexanderrichards commented 2 weeks ago

I’m having trouble understanding what autopep8 is and how it applies to my pull request. Could you please explain it, or if possible, arrange a meeting sometime this week to discuss?

so PEP8 is the python style guidelines for coding. Autopep8 is a tool that when run over a codebase will convert all files to the PEP8 standard. As part of our CI/CD pipeline we run an action that automatically runs this autopep8 tool over the files that you have modified and creates a new branch/pull request with the changes necessary to meet the PEP8 standard.

Hopefully that is clear. If not, I can have a separate conversation with you or alternatively you could probably join our Monday morning (UK time) meeting.

saadkhi commented 1 week ago

I’m having trouble understanding what autopep8 is and how it applies to my pull request. Could you please explain it, or if possible, arrange a meeting sometime this week to discuss?

so PEP8 is the python style guidelines for coding. Autopep8 is a tool that when run over a codebase will convert all files to the PEP8 standard. As part of our CI/CD pipeline we run an action that automatically runs this autopep8 tool over the files that you have modified and creates a new branch/pull request with the changes necessary to meet the PEP8 standard.

Hopefully that is clear. If not, I can have a separate conversation with you or alternatively you could probably join our Monday morning (UK time) meeting.

Okay, I'll join the meeting and then will solve the autopep8 issue after meeting as soon as possible.

alexanderrichards commented 1 week ago

Okay, I'll join the meeting and then will solve the autopep8 issue after meeting as soon as possible.

👍🏻 The meeting is in 17 mins. Do you have the connection details? If you are on the mattermost channel there is a link at the top

egede commented 1 week ago

So looking in the coverage of the testing I indeed see that the code that you have implemented is never executed. So in fact the entire runMonitoring function in Local_GangaMC_Service.py can be removed.

egede commented 1 week ago

So the real question now is what the new test files are actually testing. At the moment they just look for runMonitoring returning True, but they should be extended to see if the jobs in question are actually monitored (and others not). It could be the argument is just ignored. Sorry @saadkhi but this is turning more complicated that we realised and we probably gave you the wrong advise to begin with.

saadkhi commented 1 week ago

So the real question now is what the new test files are actually testing. At the moment they just look for runMonitoring returning True, but they should be extended to see if the jobs in question are actually monitored (and others not). It could be the argument is just ignored. Sorry @saadkhi but this is turning more complicated that we realised and we probably gave you the wrong advise to begin with.

Hi @egede, thank you for your guidance. Initially, I created the test cases based on the Local_GangaMC_Service implementation in Ganga, but encountered errors during your testing process. After that, I noticed the runMonitoring functionality in the MonitoringService and adjusted the test cases accordingly. One more things, just tell that the issue is from my side thats why code is not merged.

I’ll continue to explore the Ganga project and improve my contributions alongside the team. I appreciate your support and feedback. Thank you again!