search

scipy-conference / scipy_proceedings

Tools used to generate the SciPy conference proceedings
Other
223 stars 512 forks source link

Paper: Scikit-build-core: A modern build-backend for CPython C/C++/Fortran/Cython extensions #939

Open henryiii opened 1 month ago

henryiii commented 1 month ago

First draft. CC @jcfr and @thewtex.

Editor: Chris Calloway @cbcunc

Reviewers:

github-actions[bot] commented 1 month ago

Curvenote Preview

Directory Preview Checks Updated (UTC)
papers/henry_schreiner 🔍 Inspect ✅ 32 checks passed (4 optional) Jul 3, 2024, 4:50 PM
ameyxd commented 1 month ago

Hi @henryiii thanks for your submission! Can you update the DOI links to run checks?

jcfr commented 1 month ago

Based on example mentioned in this post, the PEPs referenced in the checks have been cited^1 using a techreport entry.

That said, I am not clear which DOI link to use in that particular case.

Is there way to add exceptions to allow the check to complete ?

_From the instructions_:

All citations that have DOIs should include those DOIs in the paper's references sectio

ameyxd commented 1 month ago

@jcfr, @henryiii you may be able to add citation keys you want to ignore if their DOIs don't exist in myst.yml under error_rules:

  - rule: doi-exists
    severity: ignore
    keys:
      - abc
      - def01
cbcunc commented 3 weeks ago

As @jcfr is a co-author, I went ahead and committed his review suggestions in order to get the paper to pass all checks so that the paper is ready for review by the assigned reviewers. Thanks @jcfr and I hope to see you in Tacoma.

jcfr commented 3 weeks ago

Thanks @cbcunc for the help :pray:

And yes, also looking forward to see you in Tacoma. @thewtex will also be here.

henryiii commented 3 weeks ago

Thanks! @jcfr, I meant to give you permissions here, I can add them if you don’t already have them.

henryiii commented 3 weeks ago

Added both you and @thewtex, sorry I forgot!

cbcunc commented 3 weeks ago

Review reminders sent to @aterrel and @MooersLab

aterrel commented 3 weeks ago

Hello! I'll have some notes on the review this week, been on vacation and travel the last few.

MooersLab commented 2 weeks ago

Review of Scikit-build-core: A modern build-backend for CPython C/C++/Fortran/Cython extensions

General Comments

Hi!

I apologize to the authors for my tardy response.

I am new to Python extension building: I am not a domain expert. Nonetheless, this paper is well-organized, precise, clear, and concise. After reading this paper, I want to use scikit-build-core to build a Python package,

Do software names need to be in italics or bold? Some package names are in different font, but the use of the different font does seem to me to be not consistent.

Some very minor copy-editing changes to suggest.

Introduction

I appreciate the respect that the authors show for the Oxford comma! Yah! You can replace the first comma in the first sentence of paragraph two of the Introduction with an EM-dash (--) or colon to avoid confusion with the four other commas in the sentence.

In the second sentence, I suggest inserting "new system" between "this" and "provides"

History of Packaging

Scikit-build (classic)

last line: awkward array is written Awkward Array by the developers.

Integration with external packages

Collapse this section into a single paragraph. The first two sentences can be combined and rewritten as one sentence.

Dual editable modes with automatic recompile.

Second to last sentence: replace since with because. Since is a weasel word when used to refer to causation. Just use because to be more direct.

The configuration system

First paragraph, last sentence: JSONSchema needs to be separated into two words.

Third paragraph, last sentence: replace since with because Third paragraph, last sentence: "yet and" is confusing. Rewrite for clarity.

Statistics

Two paragraph: There is a (TODO). Is it done?

aterrel commented 2 weeks ago

Independent Review Report

Reviewer: Andy Terrel

Department/Center/Division: Compute Products

Institution/University/Company: NVIDIA

Field of interest / expertise: Computer Science / Computational Math

Country: USA

Article reviewed: Scikit-build-core: A modern build-backend for CPython C/C++/Fortran/Cython extensions

GENERAL EVALUATION

Please rate the paper using the following criteria (please use the abbreviation to the right of the description)::

below doesn't meet standards for academic publication meets meets or exceeds the standards for academic publication n/a not applicable

SPECIFIC EVALUATION

For the following questions, please respond with 'yes' or 'no'. If you answer 'no', please provide a brief, one- to two-sentence explanation.

Yes

Yes. The article does a good job of explaining the history and current state of building python software then gives a small example for reference.

Yes, a coherent build backend that is used by numerous project is both a worthy goal and a technical achievement.

Yes

Yes

Yes. Although a few phrases are not fact but supposition:

"Python has a long history" I mean what does long mean not nearly as long as FORTRAN or the Euler equation. "packaging isn’t something that was considered important" that may be the feeling of the community but what facts are supporting this statement. "Leading to packages containing large numbers of unrelated modules (SciPy) to reduce the number of packages one had to figure out how to install" -> I don't think this was the motivation for SciPy or that the modules are "unrelated"

Additionally, if you are going to talk about wheel formats you should also talk about conda packages which predate the wheel and are widely used by the community. Especially since you include it in your figure.

If one is going to call out Rapids for "unusual requirements" and breaking PEP 621, it would be good to expound on those unusual requirements.

Yes

Yes, but claims that it is "easiest" and "first" on minor features (like cli builds and free-threading) are noise rather than the true engineering feat.

I think you need to include conda in your history section as it was a prior work solving a very similar problem.

No.

henryiii commented 1 week ago

Working on this now!

henryiii commented 1 week ago

@aterrel

I don't think this was the motivation for SciPy

"unrelated" is incorrect, I wanted to imply they didn't need to be together (not interdependent?). I think "distinct" is better. But otherwise I think this is exactly how Travis Oliphant describes SciPy ("His baby") in https://www.youtube.com/watch?v=gFEE3w7F0ww&t=1372s. It was the merging of several packages for ease of distribution. Ahh, that's also described in the SciPy paper, will move the reference up.

if you are going to talk about wheel formats you should also talk about conda packages

Conda packages just run pip install . which itself builds a wheel then installs it. Conda is just a general purpose (not just Python) redistribution like Enthought that is just also modular. If you have to discuss conda when discussing wheels, why not every other package manager? Conda happens to have a lot of Python in its history, but it's a redistribution.

I do think it make sense to cover conda in the paragraph before, though, especially since it came first (which I knew but had somewhat forgotten).

it would be good to expound on those unusual requirements

I thought that's exactly what I did in the following sentences? They need to change the name of the package based on the CUDA version it is compiled with and inject modified dependencies? I can expand but not sure what more to say. I did add a sentence about the discussions on this at PyCon 24's packaging summit.

but claims that it is "easiest" and "first" on minor features (like cli builds and free-threading)

The word [Ee]siest doesn't appear in the document. CLI builds - if you mean building without build or pip, but from some custom CLI tool, aren't supported yet, while maturin does support this, so we better not claim to be first there!

I removed the (if not the first) for free-threaded builds. But I wouldn't call free-threading a minor feature - it's the largest change to CPython ever. It's the first feature to force multiple builds since wide unicode, and it breaks the Stable ABI.

conda in your history section as it was a prior work solving a very similar problem

It's there now, but it doesn't solve a similar problem at all. Conda can't build anything by itself; it uses your build system (including scikit-build-core) and distributes binary that don't require building. Ahh, what you mean is you can install via cmake commands instead of python ones, and if CMake puts things in the right spots with the right names, it can be a Conda-only distribution. Similar to how root on conda-forge works. But probably missing a .dist-info folder. Okay, fair, adding a paragraph.

henryiii commented 1 week ago

I think I addressed everything except the package names, should they be package or package?

henryiii commented 1 week ago

Ah, and sorry about the TODO. It takes several hours to run the statistics, and I started it, wrote the TODO, then completely forgot about it.

aterrel commented 5 days ago

@henryiii Thanks for addressing my comments. I think adding these small details to the great work the team has done will only strengthen the paper. I look forward to seeing the presentation.