Migrate Utility functions to core#2406
Draft
egede wants to merge 11 commits intoganga-devs:developfrom
Draft
Conversation
Member
Author
|
@Ishaan-Chandak I just created a PR from your branch as it makes it easier to comment on the code. |
egede
commented
Mar 13, 2025
Member
Author
egede
left a comment
There was a problem hiding this comment.
Try to implement the changes suggested here. If that then removes the warnings, then the entire code base should be changed to use the new import location.
It would be good to write test cases for all the different utility functions as well. If you decide to do this, the tests should utilise the mock system to avoid actually submitting and running any jobs as part of the tests.
Member
Author
|
Another comment is that we should make sure that the old import location still works but emits a deprecation warning. While the code is not formally part of the Ganga API, it will have a number of external users. |
egede
commented
Mar 13, 2025
| import GangaCore.Core | ||
| from GangaCore.Core.GangaRepository import getRegistry | ||
| from GangaCore.Core.InternalServices.ShutdownManager import _protected_ganga_exitfuncs | ||
| from ganga.GangaCore.Utility.Config.Config import ConfigError |
Member
Author
There was a problem hiding this comment.
Is ConfigError ever used? If not, we can delete this line.
Contributor
There was a problem hiding this comment.
no it was not being used,
I have deleted this
Member
Author
|
OK, so I guess the final thing that is required is to change all the imports in the testing code. I give a list below. I think a global search and replace with the new location should work. |
egede
commented
Mar 13, 2025
Comment on lines
+4
to
+9
| warnings.warn( | ||
| "The module 'ganga.GangaTest.Framework.util' is deprecated and will be removed in a future release. " | ||
| "Please use 'ganga.GangaCore.Utility.job_monitoring' instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2 | ||
| ) |
Member
Author
There was a problem hiding this comment.
Did you check that this is actually working
Contributor
There was a problem hiding this comment.
Yes, I did test this. I tested the read_file functionality. I created a python script and a dummy read file and checked by importing the read_file function from GangaTest.framework.utils.
It did work correctly.
egede
commented
Mar 24, 2025
Comment on lines
+128
to
+135
| import unittest | ||
| failureException = unittest.TestCase.failureException | ||
|
|
||
| try: | ||
| from GangaCore.Utility.Config import getConfig | ||
| config = getConfig('TestingFramework') | ||
| except BaseException: # if we are outside Ganga, use a simple dict | ||
| config = {} No newline at end of file |
Member
Author
There was a problem hiding this comment.
This is not the right way to solve the problem of the failing tests. The code in GangaCore should not depend on unittest and should not mention TestingFramework. So the required change has to be made in the failing tests instead.
Member
Author
|
OK, so I think I can see two issues here with TestVirtualizationDocker.py
|
Contributor
|
As suggested, I have moved the test to the mentioned location and also changed the import for sleep_until_completed. |
Member
Author
|
Wow, the latest round of testing shows that we have seriously managed to break something here. Quite likely it is all related to the missing |
Contributor
|
The error in the testing is now regarding X.509 certificate expiry. Like in this workflow we can see that the expiry is due in 8 days. Can replacing the certificate solve this issue ? |
Member
Author
I solved this problem and will restart the tests now. I do not think that is the full issue though. |
Contributor
|
Yup, This error is similar ro the one that we faced earlier. |
Member
Author
|
OK, looked at this a bit closer. We in fact do need the |
Contributor
|
I feel the if statement present before was neccessary to prevent the errors that are appearing now, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.