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

change _Pragma to #pragma #18379

Merged
merged 7 commits into from
May 27, 2020
Merged

change _Pragma to #pragma #18379

merged 7 commits into from
May 27, 2020

Conversation

yajiedesign
Copy link
Contributor

Description

change _Pragma to #pragma
_Pragma not work with msvc

Checklist

Essentials

  • 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-gpu, edge, clang, miscellaneous, windows-cpu, centos-cpu, centos-gpu, unix-cpu, website, sanity, unix-gpu]


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

#18374

@szha szha requested a review from zhreshold May 21, 2020 18:33
@zhreshold
Copy link
Member

#pragma won't work in a macro, what's the version of msvc? At least 2019 supports _Pragma

@@ -150,7 +150,7 @@ class StackBatchify : public BatchifyFunction {
}
int sbs = static_cast<int>(bs);
MSHADOW_TYPE_SWITCH_WITH_BOOL(dtype, DType, {
_Pragma("omp parallel for num_threads(bs)")
#pragma omp parallel for num_threads(bs)
Copy link
Member

Choose a reason for hiding this comment

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

use __pragma for msvc if it does not support _Pragma

Copy link
Member

Choose a reason for hiding this comment

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

We can unify the macro like

#ifndef _MSC_VER
#define __pragma(id) _Pragma(#id)
#endif

@yajiedesign
Copy link
Contributor Author

I'll try upgrading vs

@yajiedesign
Copy link
Contributor Author

Cannot be resolved by upgrade vs.

@yajiedesign
Copy link
Contributor Author

@mxnet-bot run ci [windows-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [windows-cpu]

@szha szha requested a review from zhreshold May 22, 2020 16:22

#ifdef _MSC_VER
#if _MSC_VER < 1925
#define omp_parallel __pragma(omp parallel for num_threads(bs))
Copy link
Member

Choose a reason for hiding this comment

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

Can you make bs argument of the macro?

Copy link
Contributor

@leezu leezu left a comment

Choose a reason for hiding this comment

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

Could you clarify why CI test using MSVC 2019 work well but you run into issues on the CD? I think the MSVC 2019 version is the minimum supported version for MXNet 2, so the correct step may be to update the MSVC for CD?

As long as we test MSVC 2019 on CI, any new commit may break the CD again as long as you use an older MSVC on the CD?

@yajiedesign
Copy link
Contributor Author

@leezu Because CUDA. The old version of CUDA does not support the new version of msvc.
CI is only one CUDA version. but CD requires multiple versions of CUDA. cannot upgrade MSVC.

@leezu
Copy link
Contributor

leezu commented May 23, 2020

Thanks for clarifying. In that case, should we either drop the old Cuda versions for MXNet 2 or switch the CI to an older MSVC version? What do you think?

@leezu
Copy link
Contributor

leezu commented May 23, 2020

(Soon Cuda 11 will be released and we'll drop Cuda 9 support in any case)

@yajiedesign
Copy link
Contributor Author

Keep the MSVC of CI compatible with the minimum supported CUDA version.
Then upgrade MSVC when drop CUDA version support.

@leezu
Copy link
Contributor

leezu commented May 23, 2020

Yes, that's an option. I'm not convinced though that only fixing this current incompatibility without introducing any mechanism of preventing future problems is helpful.
So if you think we need to support MSVC 2017, then let's also turn it on in the CI. If not, then let's just use MSVC 2019 on the CD. But supporting MSVC 2017 without having any CI checks would just cause unnecessary problems down the road (with the CD breaking often)

@yajiedesign
Copy link
Contributor Author

@mxnet-bot run ci [macosx-x86_64]

@mxnet-bot
Copy link

None of the jobs entered are supported.
Jobs entered by user: [macosx-x86_64]
CI supported Jobs: [edge, windows-cpu, centos-cpu, website, windows-gpu, miscellaneous, centos-gpu, unix-cpu, clang, unix-gpu, sanity]

@yajiedesign
Copy link
Contributor Author

@zhreshold

@zhreshold zhreshold merged commit 2806212 into apache:master May 27, 2020
@leezu
Copy link
Contributor

leezu commented May 27, 2020

@yajiedesign please clarify how you plan to address the CI CD discrepancy?

AntiZpvoh pushed a commit to AntiZpvoh/incubator-mxnet that referenced this pull request Jul 6, 2020
* change _Pragma to #pragma._Pragma not work with msvc

* new plan

* fix lint

* add arg

* fix

* fix lint

* fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants