Migrate Utility functions to core #2406
Draft
+247
−240
Conversation
This file contains 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
egede
temporarily deployed
to
Integrate Pull Request
GitHub Actions
Inactive
|
@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
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.
|
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. |
Ishaan-Chandak
temporarily deployed
to
Integrate Pull Request
GitHub Actions
Inactive
egede
commented
Mar 13, 2025
ganga/ganga/__init__.py
Outdated
| @@ -10,6 +10,7 @@ | |||
| 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 | |||
There was a problem hiding this comment.
Is ConfigError ever used? If not, we can delete this line.
There was a problem hiding this comment.
no it was not being used,
I have deleted this
Ishaan-Chandak
requested a deployment
to
Integrate Pull Request
GitHub Actions
Waiting
|
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
ganga/GangaTest/Framework/utils.py
Outdated
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 | ||
| ) |
There was a problem hiding this comment.
Did you check that this is actually working
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.
Ishaan-Chandak
temporarily deployed
to
Integrate Pull Request
GitHub Actions
Inactive
Ishaan-Chandak
requested a deployment
to
Integrate Pull Request
GitHub Actions
Waiting
egede
commented
Mar 24, 2025
Comment on lines
128
to
135
| 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 = {} |
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.
|
OK, so I think I can see two issues here with TestVirtualizationDocker.py
|
Ishaan-Chandak
temporarily deployed
to
Integrate Pull Request
GitHub Actions
Inactive
|
As suggested, I have moved the test to the mentioned location and also changed the import for sleep_until_completed. |
|
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 |
|
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 ? |
I solved this problem and will restart the tests now. I do not think that is the full issue though. |
|
Yup, This error is similar ro the one that we faced earlier. |
|
OK, looked at this a bit closer. We in fact do need the |
Ishaan-Chandak
requested a deployment
to
Integrate Pull Request
GitHub Actions
Waiting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.