Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix copy manager functionality so it works under Python >= 3.8 on OS X #818

Merged
merged 6 commits into from
Jan 20, 2022

Conversation

Kami
Copy link
Contributor

@Kami Kami commented Jan 20, 2022

This pull request fixes copy manager functionality and related tests so they work on OS X when using Python >= 3.8.

In Python 3.8, local scoped objects can't be pickled anymore so the code fails. The solution is to use _SharedObjectManager class definition which is used for communication and is pickled by the multiprocessing framework to a top level scope.

Related links:

In Python 3.8, local objects can't be pickled anymore which means we
need to move that class definition to a top module level scope so it can
be pickled by the multiprocess functionality.

See https://gist.github.com/Kami/8386c79d2db7c95329e6c182ec639f49 and
spack/spack#14102 for more details.
@Kami Kami requested a review from ArthurKamalov January 20, 2022 10:35
@ArthurKamalov
Copy link
Contributor

@Kami The copying manager-related test still fails. CPython switched to the spawn method from the fork in 3.8, I guess that's why multiprocessing.Manager can not work properly. As a quick workaround, we can disable multiprocess workers to threaded ones as we did for Windows.

@Kami
Copy link
Contributor Author

Kami commented Jan 20, 2022

@ArthurKamalov Thanks. Tests were passing locally after I made this change, but it looks like I missed the spawn change which is also needed.

For now, I will indeed likely looking into skipping those tests on OS X.

@Kami
Copy link
Contributor Author

Kami commented Jan 20, 2022

@ArthurKamalov Pushed a change to also skip multiprocess copying manager worker tests under OS X.

On a related note, does the same issue also exists on Linux? If so, we will want to fix this so we can still use multiprocess functionality on Linux under Python >= 3.8.

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #818 (fdebe18) into master (e1d2a25) will decrease coverage by 0.04%.
The diff coverage is 62.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
- Coverage   78.42%   78.38%   -0.04%     
==========================================
  Files         164      164              
  Lines       37884    37885       +1     
  Branches     4434     4434              
==========================================
- Hits        29709    29693      -16     
- Misses       7090     7098       +8     
- Partials     1085     1094       +9     
Impacted Files Coverage Δ
scalyr_agent/copying_manager/worker.py 77.28% <56.52%> (+0.04%) ⬆️
.../copying_manager_tests/copying_manager_new_test.py 97.30% <100.00%> (ø)
...unit/copying_manager_tests/copying_manager_test.py 97.82% <100.00%> (ø)
...ing_manager_tests/copying_manager_unittest_test.py 95.30% <100.00%> (ø)
...pying_manager_tests/copying_manager_worker_test.py 97.56% <100.00%> (ø)
scalyr_agent/test_base.py 70.64% <0.00%> (-1.28%) ⬇️
...s/unit/builtin_monitors/kubernetes_monitor_test.py 98.13% <0.00%> (-1.12%) ⬇️
scalyr_agent/copying_manager/copying_manager.py 83.33% <0.00%> (-0.69%) ⬇️
scalyr_agent/builtin_monitors/docker_monitor.py 73.67% <0.00%> (-0.60%) ⬇️
scalyr_agent/util.py 82.20% <0.00%> (-0.10%) ⬇️
... and 4 more

@ArthurKamalov
Copy link
Contributor

@ArthurKamalov Pushed a change to also skip multiprocess copying manager worker tests under OS X.

On a related note, does the same issue also exists on Linux? If so, we will want to fix this so we can still use multiprocess functionality on Linux under Python >= 3.8.

I pushed the commit which enables smoke and unit tests for python 3.8. The smoke test seems like working, but the unit test hasn't appeared in CircleCI for some reason.

Copy link
Contributor

@ArthurKamalov ArthurKamalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Kami Kami merged commit 1d9aa23 into master Jan 20, 2022
@Kami Kami deleted the copy_manager_osx_py38_fix branch January 20, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants