Skip to content

Disabling Python 3.6 and Fixing 3.9 Unit Tests in CI Workflow#459

Merged
mambelli merged 3 commits intoHEPCloud:masterfrom
namrathaurs:disable_py36_unittests
Jun 7, 2023
Merged

Disabling Python 3.6 and Fixing 3.9 Unit Tests in CI Workflow#459
mambelli merged 3 commits intoHEPCloud:masterfrom
namrathaurs:disable_py36_unittests

Conversation

@namrathaurs
Copy link
Contributor

@namrathaurs namrathaurs commented May 4, 2023

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,

  • for 3.9 python unit tests, there were at least a few libraries, one of them being pandas, that were surely at a higher version in the CI workflow than on the dev environment (something that Kyle had previously pointed out)
  • for python 3.6 unit tests, the error message in the CI workflow said “Version 3.6 was not found in the local cache. Error: The version '3.6' with architecture 'x64' was not found for Ubuntu 22.04.” The assumption was that python 2.6 is no longer supported in Ubuntu 22.04 (64-bit architecture) and Marco confirmed that this was indeed the case.

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):

  1. 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'

    • The version of 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 in setup.py, which is pandas >= 1.1.5; python_version >= '3.7', pandas 2.0.1 was installed for the CI workflow.
    • For pandas versions 1.1.5 to 1.5.0 (exclusive), the attribute UndefinedVariableError was defined in the pandas.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 the pandas.errors module.
    • Possible solutions are: (1) freezing the version requirement of pandas, to be installed, to >=1.5.3 for python >= 3.7, or (2) modify the underlying python script to import the attribute 'UndefinedVariableError' based on the version of pandas installed.
    • Opted to go forward with updating pandas version to, at least, be 1.5.3 when python >=3.7 as we need to ensure the use of latest version of pandas moving forward as we do not want to run into similar issues again in the future.
  2. 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 groupby with 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 an AssertionError in the following unit tests:

    .../tests/glideinwms/publishers/test_decisionenginemonitor.py::test_create_invalidate_constraint
    .../tests/glideinwms/publishers/test_fe_group_classads.py::test_create_invalidate_constraint
    .../tests/glideinwms/publishers/test_glideclientglobal.py::test_create_invalidate_constraint

    • In python scripts corresponding to those unit tests, the line for collector_host, request_group in requests_df.groupby(["CollectorHost"]): is what was causing the warning to be thrown.
    • Upon testing locally on my machine, regardless of python 3.9.x, pandas 1.5.3 throws the warning but was successful with assert statements in the unit tests
    • Because the version of pandas, installed as part of the CI workflow, was >= 2.0.x, this led to the assert 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.
    • So, when using pandas 2.0.x, the FutureWarning was no longer thrown and keys were being returned as a tuple of length 1 instead of strings, which is what was observed when the CI workflow ran python 3.9 unit tests.
    • Changed the grouper to a string (in contrast to a list with the string). That is, for ... in ...("col_name"): instead of for ... 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 the assert statements.
  3. Another message observed was AttributeError: 'DataFrame' object has no attribute 'append'

    • The CI workflow uses pandas >= 1.5.3 but since pandas 1.4.0, pandas.DataFrame.append() has been deprecated and with pandas >= 2.0.0, the append method has been completely removed. So, code that references append() reports an AttributeError.
    • As of pandas 2.0.x, the concat() method can be used to concatenate multiple dataframes and this was reflected in the file src/decisionengine_modules/glideinwms/resource_dist_plugins.py

@namrathaurs namrathaurs self-assigned this May 4, 2023
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Patch coverage: 83.33% and project coverage change: +0.16 🎉

Comparison is base (590bede) 47.20% compared to head (35f6d45) 47.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
+ Coverage   47.20%   47.36%   +0.16%     
==========================================
  Files          54       54              
  Lines        2896     2903       +7     
  Branches      523      497      -26     
==========================================
+ Hits         1367     1375       +8     
+ Misses       1428     1427       -1     
  Partials      101      101              
Flag Coverage Δ
python-3.6 ?
python-3.8 47.33% <83.33%> (?)
python-3.9 47.36% <83.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
setup.py 0.00% <ø> (ø)
src/decisionengine_modules/NERSC/util/newt.py 35.21% <ø> (ø)
...ne_modules/glideinwms/transforms/job_clustering.py 65.30% <50.00%> (ø)
...les/glideinwms/publishers/decisionenginemonitor.py 88.23% <100.00%> (ø)
...modules/glideinwms/publishers/fe_group_classads.py 89.09% <100.00%> (ø)
...modules/glideinwms/publishers/glideclientglobal.py 88.23% <100.00%> (ø)
...engine_modules/glideinwms/resource_dist_plugins.py 73.07% <100.00%> (ø)

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@namrathaurs namrathaurs force-pushed the disable_py36_unittests branch from a5dd4d2 to 4a59acc Compare May 11, 2023 22:01
@namrathaurs namrathaurs changed the title Disabling Python 3.6. Unit Tests in CI Workflow Disabling(Fixing) Python 3.6(3.9) Unit Tests in CI Workflow May 19, 2023
@namrathaurs namrathaurs changed the title Disabling(Fixing) Python 3.6(3.9) Unit Tests in CI Workflow Disabling Python 3.6 and Fixing 3.9 Unit Tests in CI Workflow May 19, 2023
@namrathaurs namrathaurs reopened this May 19, 2023
@namrathaurs namrathaurs force-pushed the disable_py36_unittests branch from d77a98f to b4c2122 Compare May 19, 2023 22:51
@namrathaurs namrathaurs requested a review from mambelli May 19, 2023 23:41
@namrathaurs namrathaurs marked this pull request as ready for review May 19, 2023 23:41
@mambelli
Copy link
Contributor

mambelli commented May 21, 2023

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.

@namrathaurs
Copy link
Contributor Author

@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:

  • The first two warnings can be ignored since they are concerning xdist and pytest which may not directly affect the Decision Engine code
  • The third warning seems important to be fixed but I'm not sure if that's something we can address as I'm not aware of the python script where the already deprecated functionality is still being used
  • The fourth warning is in a python script that is a part of the GlideinWMS libraries, which I believe needs to be fixed at the GlideinWMS level, for Decision Engine to reflect the changes

Please confirm on which of the above reported warnings needs immediate attention/addressing.

@namrathaurs namrathaurs requested a review from mambelli May 22, 2023 20:35
@mambelli
Copy link
Contributor

../../../../../../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.

@mambelli
Copy link
Contributor

../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

@mambelli
Copy link
Contributor

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.

@namrathaurs
Copy link
Contributor Author

namrathaurs commented May 26, 2023

UPDATE (post PR review)

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.

- disabled the workflow to remove the unit tests completely for python 3.6 upon confirmation that python 3.6 is no longer supported on Ubuntu 22.04 (used in the CI workflow)
- updated the requirement for the version of pandas to be at least 1.5.3 when python installed is >=3.7 because 'pandas.core.computation.ops.UndefinedVariableError` has been exposed via `pandas.errors` module starting pandas 1.5.0
- changed the groupby grouper to use the string directly
-- pandas version >= 1.5.0 throws FutureWarning that a length 1 tuple will be returned when iterating over the values returned with a grouper equal to a list of length 1.
-- when a single grouper is used, the grouper can be specified without the list but just the string, hence avoiding the warning
- changed the deprecated functionality for append method
-- replaced append method with the concat method as append functionality was deprecated since pandas 1.4.0 and completely removed in 2.0.0.
@namrathaurs namrathaurs force-pushed the disable_py36_unittests branch from ab47dfc to fa40116 Compare May 26, 2023 20:09
- updated to running unit tests for 3.8 instead of the initial suggestion to switch to 3.7 as 3.7 will soon be reaching EOL (June 2023).
- commented the timeout option in the toml file to avoid the DeprecationWarning associated with it during the unit tests on python 3.9
- changed the glideinwms ref from `branch_v3_9` to `master` in the yaml file for the CI as the latter is the running branch for GWMS v3.9.x
@namrathaurs
Copy link
Contributor Author

namrathaurs commented May 26, 2023

../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.

@mambelli mambelli merged commit 07292a4 into HEPCloud:master Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants