Closed namrathaurs closed 1 year ago
Patch coverage: 83.33
% and project coverage change: +0.16
:tada:
Comparison is base (
590bede
) 47.20% compared to head (35f6d45
) 47.36%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I requested a couple of changes to complete the migration away of Python 3.6 and I'm opening an issue in decisionengine. Good job in finding the multiple changes. Please check also if there are other bug changes in Pandas 2.0 that may affect the code and inspect the decision_engine modules code in case the tests missed something that is silently changing the behavior.
@mambelli, after the above requested changes were made, Python 3.9 unit tests are running successfully, however, there are some warnings being thrown. There are four warnings reported, of which three of them are DeprecationWarning
. Also, one of the deprecation warnings is caused by the underlying code in GlideinWMS libraries. Following is the complete list of them:
=============================== warnings summary ===============================
../../../../.local/lib/python3.9/site-packages/xdist/plugin.py:252
../../../../.local/lib/python3.9/site-packages/xdist/plugin.py:252
../../../../.local/lib/python3.9/site-packages/xdist/plugin.py:252
../../../../.local/lib/python3.9/site-packages/xdist/plugin.py:252
../../../../.local/lib/python3.9/site-packages/xdist/plugin.py:252
/home/runner/.local/lib/python3.9/site-packages/xdist/plugin.py:252: DeprecationWarning: The --rsyncdir command line argument and rsyncdirs config variable are deprecated.
The rsync feature will be removed in pytest-xdist 4.0.
config.issue_config_time_warning(warning, 2)
../../../../.local/lib/python3.9/site-packages/_pytest/config/__init__.py:1[233](https://github.com/HEPCloud/decisionengine_modules/actions/runs/5049395319/jobs/9058794739#step:14:234)
../../../../.local/lib/python3.9/site-packages/_pytest/config/__init__.py:1233
../../../../.local/lib/python3.9/site-packages/_pytest/config/__init__.py:1233
../../../../.local/lib/python3.9/site-packages/_pytest/config/__init__.py:1233
../../../../.local/lib/python3.9/site-packages/_pytest/config/__init__.py:1233
/home/runner/.local/lib/python3.9/site-packages/_pytest/config/__init__.py:1233: PytestConfigWarning: Unknown config option: timeout
self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")
../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2
../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2
../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2
../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.10 it will stop working
from collections import MutableMapping
../glideinwms/creation/lib/matchPolicy.py:17
../glideinwms/creation/lib/matchPolicy.py:17
../glideinwms/creation/lib/matchPolicy.py:17
../glideinwms/creation/lib/matchPolicy.py:17
/home/runner/work/decisionengine_modules/decisionengine_modules/glideinwms/creation/lib/matchPolicy.py:17: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
import imp
-- Docs: https://docs.pytest.org/en/stable/warnings.html
Here's what I understand:
xdist
and pytest
which may not directly affect the Decision Engine codePlease confirm on which of the above reported warnings needs immediate attention/addressing.
../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 ../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 ../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 ../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 /opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.10 it will stop working from collections import MutableMapping
Send this to the condor team. It is a very easy fix. Maybe an issue on github or check on their site about how to submit bugs.
../glideinwms/creation/lib/matchPolicy.py:17 ../glideinwms/creation/lib/matchPolicy.py:17 ../glideinwms/creation/lib/matchPolicy.py:17 ../glideinwms/creation/lib/matchPolicy.py:17 /home/runner/work/decisionengine_modules/decisionengine_modules/glideinwms/creation/lib/matchPolicy.py:17: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses import imp
This one is not urgent but needs to be fixed. You can quickly check how we use imp and if there is a one-to-one replacement in importlib. If yes, do the replacement. If not open an issue
Issue 1 I think is external. 2 may be something we add in the configuration of the unit tests. Maybe the timeout option was supported and is no more or we specify it wrongly? Please check.
Issue 1 I think is external. 2 may be something we add in the configuration of the unit tests. Maybe the timeout option was supported and is no more or we specify it wrongly? Please check.
warning#1 has been ignored as suggested since xdist
is external to GlideinWMS and needs to be fixed by its maintainers. To avoid warning#2, the timeout
option can be removed altogether, as suggested here, which has been added to the latest commit.
../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 ../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 ../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 ../../../../../../opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2 /opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/classad/_expression.py:2: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.10 it will stop working from collections import MutableMapping
Send this to the condor team. It is a very easy fix. Maybe an issue on github or check on their site about how to submit bugs.
Warning#3, as suggested, was reported to the HTCondor team and Cole Bollig, from their team, informed that the classad
library reported in the warning is actually not the native HTCondor Python Bindings but is a third party PyPi project; the linked github repository for this is a pure python implementation of classad. He also informed that he did check through their code base for _expression.py
and could not find the script under question. So, we may have to just ignore that warning being thrown for the classad/_expression.py
script for now until the decision is made whether to continue having this or completely remove it from our system requirements.
../glideinwms/creation/lib/matchPolicy.py:17 ../glideinwms/creation/lib/matchPolicy.py:17 ../glideinwms/creation/lib/matchPolicy.py:17 ../glideinwms/creation/lib/matchPolicy.py:17 /home/runner/work/decisionengine_modules/decisionengine_modules/glideinwms/creation/lib/matchPolicy.py:17: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses import imp
This one is not urgent but needs to be fixed. You can quickly check how we use imp and if there is a one-to-one replacement in importlib. If yes, do the replacement. If not open an issue
Regarding warning#4, upon going through a couple of SO discussions, it seemed like this would be a 1:1 replacement (meaning just changing all references of imp
to importlib
module), however, it turned out that there's a bit more that needs to be changed as pylint was found reporting the following errors with just the 1:1 replacement:
************* Module creation.lib.matchPolicy
creation/lib/matchPolicy.py:86:32: E1101: Module 'importlib' has no 'find_module' member (no-member)
creation/lib/matchPolicy.py:88:32: E1101: Module 'importlib' has no 'load_module' member (no-member)
************* Module creation.lib.matchPolicy
creation/lib/matchPolicy.py:86:32: E1101: Module 'importlib' has no 'find_module' member (no-member)
creation/lib/matchPolicy.py:88:32: E1101: Module 'importlib' has no 'load_module' member (no-member)
From the documentation for find_module and load_module, these two methods have been deprecated since Python 3.3. Unless compatibility with Python 3.3 is desired, these would need to be replaced with importlib.util.find_spec()
and importlib.util.spec_from_file_location()
, importlib.util.module_from_spec()
respectively. Even though the replacement is done with importlib
, there is another similar DeprecationWarning regarding imp
being thrown by boto
package:
../../../../.local/lib/python3.9/site-packages/boto/plugin.py:40
../../../../.local/lib/python3.9/site-packages/boto/plugin.py:40
../../../../.local/lib/python3.9/site-packages/boto/plugin.py:40
../../../../.local/lib/python3.9/site-packages/boto/plugin.py:40
/home/runner/.local/lib/python3.9/site-packages/boto/plugin.py:40: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
import imp
Since boto
is an external library, we decided to ignore the warning as this cannot be handled by us unless the maintainers of that library release a patch/update.
NOTE: This has also been documented under the GlideinWMS project issue#300. Despite the fix comes in from issue#300, this would still lead to the cascading DeprecationWarning in boto
as described above.
As part of completing my activity on the PR for making CONTINUE_IF_NO_PROXY a configurable attribute (#457), an attempt was made to push the changes to the respective branch on the decisionengine codebase. The CI workflow reported that python 3.6 and 3.9 unit tests did not pass. The suggestion was to run the unit tests locally to understand what might be causing the tests to fail.
When the unit tests were run locally, no errors were reported and all the unit tests passed. This meant that there could be a difference in the packages (dependencies) in my dev environment versus what was being installed in the environment that the CI workflow was setting up. Specifically,
The recommendation was to disable python 3.6 unit tests in the CI workflow as python 3.6 is not supported. Further for python 3.9, separately investigate further to identify the differences in dependencies (versions, especially) to understand how the fixes can be made for unit tests to be successful as it is the case when these tests are run locally.
Must complete the following [non-blocking]:
UPDATE
Following are the specific details pertaining to the investigation behind why python3.9 unit tests were failing (these are also included in the commit description):
The error message in the Github action for "Run Python 3.9 unit tests" says:
src/decisionengine_modules/glideinwms/transforms/job_clustering.py:115: AttributeError: module 'pandas.core.computation.ops' has no attribute 'UndefinedVariableError'
pandas
on my dev environment was at version 1.1.5 but pandas installed during the CI workflow was at 1.5.3. For the CI workflow, python 3.9 was installed and due to the dependency requirement defined insetup.py
, which ispandas >= 1.1.5; python_version >= '3.7'
, pandas 2.0.1 was installed for the CI workflow.UndefinedVariableError
was defined in thepandas.core.computation.ops
module. As per the release notes for pandas 1.5.0, starting with 1.5.0,UndefinedVariableError
, among others, are changed to be exposed via thepandas.errors
module.Unit tests for a few python scripts in the codebase failed with the message
FutureWarning: In a future version of pandas, a length 1 tuple will be returned when iterating over a
groupbywith a grouper equal to a list of length 1. Don't supply a list with a single grouper to avoid this warning.
This ultimately was resulting in anAssertionError
in the following unit tests:for collector_host, request_group in requests_df.groupby(["CollectorHost"]):
is what was causing the warning to be thrown.assert
statements in the unit testsassert
statements to fail. Because starting pandas 1.5.x, when using a grouper that is equal to a list of length 1, a length-1 tuple is returned rather than a single string element.for ... in ...("col_name"):
instead offor ... in ...(["col_name"]):
. This returned the keys as strings and not length-1 tuples, which is what the expected output of the unit tests were through theassert
statements.Another message observed was
AttributeError: 'DataFrame' object has no attribute 'append'
pandas.DataFrame.append()
has been deprecated and with pandas >= 2.0.0, the append method has been completely removed. So, code that referencesappend()
reports an AttributeError.concat()
method can be used to concatenate multiple dataframes and this was reflected in the filesrc/decisionengine_modules/glideinwms/resource_dist_plugins.py