- 
                Notifications
    You must be signed in to change notification settings 
- Fork 31
          Remove use of python setup.py develop/install in the project
          #2172
        
          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
base: master
Are you sure you want to change the base?
Conversation
6d87c72    to
    124f5f7      
    Compare
  
    | View rendered docs @ https://intelpython.github.io/dpctl/pulls/2172/index.html | 
| Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_29 ran successfully. | 
| Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_29 ran successfully. | 
fd45fec    to
    13409b7      
    Compare
  
    | Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_30 ran successfully. | 
13409b7    to
    32f1509      
    Compare
  
    | Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_30 ran successfully. | 
    
      
        1 similar comment
      
    
  
    | Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_30 ran successfully. | 
python setup.py develop in the projectpython setup.py develop/install in the project
      | p.add_argument( | ||
| "--debug", | ||
| default="Release", | ||
| dest="build_type", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug build produces a lot of the warning which probably has to be resolved in the follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a number of them are
icpx: warning: argument unused during compilation: '-Xsycl-target-frontend=spir64 -g0'
which will need some looking at
the remainder are
warning: comparison of integers of different signs: 'py::ssize_t' (aka 'long') and 'std::size_t'
from
assert(dst.get_size() == iter_nelems)
in sorting functions
should be a simple enough fix
00161f1    to
    989df1b      
    Compare
  
    | Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_32 ran successfully. | 
| Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_31 ran successfully. | 
| Array API standard conformance tests for dpctl=0.21.0=py310h93fe807_32 ran successfully. | 
acb4584    to
    a113d01      
    Compare
  
    | Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_33 ran successfully. | 
| Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_34 ran successfully. | 
    
      
        1 similar comment
      
    
  
    | Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_34 ran successfully. | 
| Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_35 ran successfully. | 
| pushd $d | ||
| conda activate --stack ${{ env.BUILD_ENV_NAME }} | ||
| CC=icx CXX=icpx python setup.py develop -G Ninja || exit 1 | ||
| CC=icx CXX=icpx python setup.py build_ext --inplace -G Ninja || exit 1 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONTRIBUTING.md needs to be updated also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and docs/_legacy/docfiles/user_guides/QuickStart.rst and docs/doc_sources/contributor_guides/building.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some README.md in the examples folder as well
| import sys | ||
|  | ||
| # add scripts dir to Python path so we can import _build_helper | ||
| sys.path.insert(0, os.path.abspath("scripts")) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder for what use case we need that? Simple python scripts/build_locally.py works fine without that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't work for me locally without this, essentially importing from _build_helper wouldn't work without this or an __init__.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it would be better to add __init__.py then. It looks clearer.
| dest="no_level_zero", | ||
| action="store_true", | ||
| default=False, | ||
| help="Disable Level Zero backend (deprecated: use --target-level-zero " | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--target-level-zero OFF does not work
to be reused in gen_docs and gen_coverage
6abda67    to
    f49a274      
    Compare
  
    f49a274    to
    1800644      
    Compare
  
    | Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_31 ran successfully. | 
| Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_31 ran successfully. | 
| Array API standard conformance tests for dpctl=0.22.0dev0=py310h93fe807_31 ran successfully. | 
| f"-DCMAKE_C_COMPILER:PATH={c_compiler}" if c_compiler else "", | ||
| f"-DCMAKE_CXX_COMPILER:PATH={cxx_compiler}" if cxx_compiler else "", | ||
| f"-DDPCTL_ENABLE_L0_PROGRAM_CREATION={'ON' if level_zero else 'OFF'}", | ||
| f"-DDPTL_ENABLE_GLOG:BOOL={'ON' if glog else 'OFF'}", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| f"-DDPTL_ENABLE_GLOG:BOOL={'ON' if glog else 'OFF'}", | |
| f"-DDPCTL_ENABLE_GLOG:BOOL={'ON' if glog else 'OFF'}", | 
| if other_opts: | ||
| args.extend(other_opts.split()) | ||
|  | ||
| return args | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to pass a list containing empty ("") arguments to CMake?
| if args.target_hip is not None and not args.target_hip.strip(): | ||
| err("--target-hip requires an explicit architecture", "build_locally") | ||
|  | ||
| # CUDA/HIP targets | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation included a safeguard to prevent using --target-cuda together with -DDPCTL_TARGET_CUDA=...  (HIP as well) in --cmake-opts
if target_cuda is not None:
        if not target_cuda.strip():
            raise ValueError(
                "--target-cuda can not be an empty string. "
                "Use --target-cuda=<arch> or --target-cuda"
            )
        if any(opt.startswith("-DDPCTL_TARGET_CUDA=") for opt in cmake_args):
            raise ValueError(
                "Both --target-cuda and -DDPCTL_TARGET_CUDA in --cmake-opts "
                "were specified. Please use only one method "
                "to avoid ambiguity"
            )
        cmake_args += [
            f"-DDPCTL_TARGET_CUDA={target_cuda}",
        ]
Why did you decide to remove this check?
|  | ||
|  | ||
| def err(msg: str, script: str): | ||
| print(f"[{script}][error] {msg}", file=sys.stderr) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function only prints an error message.
Should we also terminate build_locally script when err() is called?
| "cxx_compiler", | ||
| "compiler_root", | ||
| ] | ||
| p.add_argument( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work
$ python scripts/build_locally.py --clean
$ ls | grep _skbuild
> _skbuild
$ ls _skbuild/linux-x86_64-3.13/
> cmake-build  cmake-install  setuptools
A warning that
python setup.py developis deprecated and will no longer be supported at the end of October, 2025 has been ongoing for some time when using dpctl build driver scripts, and can be seen in the CIThis PR proposes instead relying on
pipfor installing dpctl in scripts, and reworks the scripts to maintain use of scikit-buildIn the future, this will also simplify a transition to scikit-build-core
The PR also introduces options
--cleanand--skip-editabletobuild_locallydriver--cleanand--skip-pytesttogen_coveragedriver--cleantogen_docsdriver