Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[BUGFIX]fix python 3.8 ctypes dll load with windows #19236

Merged
merged 1 commit into from
Oct 1, 2020
Merged

[BUGFIX]fix python 3.8 ctypes dll load with windows #19236

merged 1 commit into from
Oct 1, 2020

Conversation

yajiedesign
Copy link
Contributor

Description

fix python 3.8 ctypes dll load with windows

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)

Changes

Comments

@mxnet-bot
Copy link

Hey @yajiedesign , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [windows-cpu, windows-gpu, unix-cpu, sanity, website, centos-cpu, unix-gpu, clang, edge, centos-gpu, miscellaneous]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@yajiedesign
Copy link
Contributor Author

@szha @aaronmarkham Lint doesn't know the new parameter. What should I do?

@yajiedesign
Copy link
Contributor Author

I disable it.

szha
szha reviewed Sep 27, 2020
View reviewed changes
python/mxnet/base.py Outdated Show resolved Hide resolved
@yajiedesign
Copy link
Contributor Author

run ci [windows-gpu]

@wkcn
Copy link
Member

wkcn commented Sep 29, 2020

https://docs.python.org/3/library/ctypes.html

The document shows that

The winmode parameter is used on Windows to specify how the library is loaded (since mode is ignored). It takes any value that is valid for the Win32 API LoadLibraryEx flags parameter. When omitted, the default is to use the flags that result in the most secure DLL load to avoiding issues such as DLL hijacking. Passing the full path to the DLL is the safest way to ensure the correct library and dependencies are loaded.

Could we pass the full path of DLL by lib = ctypes.CDLL(os.path.abspath(lib_path[0])) ?

@yajiedesign
Copy link
Contributor Author

@wkcn The key is to find CUDA's dll. Adjusting mxnet.dll path doesn't help.

marcoabreu
marcoabreu reviewed Sep 29, 2020
View reviewed changes
python/mxnet/base.py
if sys.version_info >= (3, 8) and os.name == "nt":
# use LOAD_WITH_ALTERED_SEARCH_PATH, For simplicity, let's just fill the numbers.
# pylint: disable=E1123
lib = ctypes.CDLL(lib_path[0], winmode=0x00000008)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the magic value available as a constant somewhere?

Copy link
Contributor Author

@yajiedesign yajiedesign Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not at the moment.So I added some comments.

@szha szha merged commit 464608c into apache:master Oct 1, 2020
chinakook pushed a commit to chinakook/mxnet that referenced this pull request Nov 17, 2020
@yajiedesign @chinakook
fix python 3.8 ctypes dll load with windows (apache#19236)
0b28afd
josephevans pushed a commit to josephevans/mxnet that referenced this pull request Oct 19, 2021
@yajiedesign @josephevans
fix python 3.8 ctypes dll load with windows (apache#19236)
55c4d5c
@josephevans josephevans mentioned this pull request Oct 19, 2021
Merged
josephevans added a commit that referenced this pull request Oct 20, 2021
@josephevans @barry-jin @yajiedesign
[v1.x] Port #20648, #20676, #19236 to v1.x (#20677)
b1ac65d 
* [v1.9.x] CI fixes (#20648)

* Update openssl package in ubuntu_core.sh (used in ubuntu 16.04 images) to avoid bug triggered by let's encrypt expired ca cert.

* Reduce number of parallel docker builds we run at once to 2, since there are multiple jobs (on different branches) running at the same time.

* Use different mirror to download binutils to avoid expired let's encrypt CA cert.

* [CI] UPgrade windows CI

* fix python 3.8 ctypes dll load with windows (#19236)

Co-authored-by: barry-jin <[email protected]>
Co-authored-by: Hu Shiwen <[email protected]>
josephevans added a commit to josephevans/mxnet that referenced this pull request Nov 3, 2021
@josephevans @barry-jin @yajiedesign
[v1.x] Port apache#20648, apache#20676, apache#19236 to v1.x (apache#…
5584430 
…20677)

* [v1.9.x] CI fixes (apache#20648)

* Update openssl package in ubuntu_core.sh (used in ubuntu 16.04 images) to avoid bug triggered by let's encrypt expired ca cert.

* Reduce number of parallel docker builds we run at once to 2, since there are multiple jobs (on different branches) running at the same time.

* Use different mirror to download binutils to avoid expired let's encrypt CA cert.

* [CI] UPgrade windows CI

* fix python 3.8 ctypes dll load with windows (apache#19236)

Co-authored-by: barry-jin <[email protected]>
Co-authored-by: Hu Shiwen <[email protected]>
josephevans added a commit that referenced this pull request Nov 3, 2021
@josephevans @barry-jin @yajiedesign
[v1.9.x] Port #20709 to v1.9.x branch (#20722)
200b284 
* [v1.x] License updates  (#20709)

* Remove Apache-2.0 license header from ONNX files that were originally BSD 3-clause licensed and properly list then under BSD 3-clause section in LICENSE.

* Remove 3rdparty/mkldnn/src/common/primitive_hashing.hpp from LICENSE under Boost License section, since it is clearly Apache 2.0 licensed.

* Clarify Caffe license to BSD 2-clause with Caffe extensions.

* Clarify Caffe license to BSD 2-clause with Caffe extensions.

* Remove duplicate file, should only be in BSD 3-clause section.

* Update LICENSE with latest from 3rdparty/mkldnn/THIRD-PARTY-PROGRAMS

* Sort files under MIT license. Remove references to generic licenses.

* Remove incorrectly added Apache header on MIT-licensed files.

* Sort lines in ASF-2.0 licensed list.

* Add license text in licenses/ to fulfill binary distribution requirements.

* Remove copyright by contributors line from ASF-licensed file.

* Fix rat-excludes and licensecheck exclude list.

* Error out and fail if license_header.py tool detects multiple licenses in a file.

* Move LayerNormCPUKernel function to own file, since it is licensed under MIT. Update LICENSE to reflect this.

* Remove LayerNormCPUKernel from layer_norm.cc, it is now in layer_norm_cpu.h.

* Add header guard, fix first line to pass lint.

* Update skywalking-eyes config based on current LICENSE, add layer_norm_cpu.h to whitelist.

* Rename license file for layer_norm_cpu.h

* Update mkldnn license text and LICENSE.

* [v1.x] Port #20648, #20676, #19236 to v1.x (#20677)

* [v1.9.x] CI fixes (#20648)

* Update openssl package in ubuntu_core.sh (used in ubuntu 16.04 images) to avoid bug triggered by let's encrypt expired ca cert.

* Reduce number of parallel docker builds we run at once to 2, since there are multiple jobs (on different branches) running at the same time.

* Use different mirror to download binutils to avoid expired let's encrypt CA cert.

* [CI] UPgrade windows CI

* fix python 3.8 ctypes dll load with windows (#19236)

Co-authored-by: barry-jin <[email protected]>
Co-authored-by: Hu Shiwen <[email protected]>

* Remove general license text files, as this is included in individual license files in licenses/*. Update license for code contributed/derived from https://github.com/msracver/Deformable-ConvNets which is now MIT licensed and update LICENSE to reflect this. Add references for original code with github revision to MIT-licensed files.

* Fix rat-excludes.

* Update skywalking-eyes config.

Co-authored-by: barry-jin <[email protected]>
Co-authored-by: Hu Shiwen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marcoabreu marcoabreu marcoabreu left review comments

@szha szha szha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yajiedesign @mxnet-bot @wkcn @szha @marcoabreu