mesonbuild / meson-python

Meson PEP 517 Python build backend
https://mesonbuild.com/meson-python/
MIT License
120 stars 59 forks source link

ENH: implement variables substitution in configuration #488

Open dnicolodi opened 10 months ago

dnicolodi commented 10 months ago

@rgommers This is now working. We would just need to plug in a function to compute ncores and some documentation.

rgommers commented 9 months ago

This is looking interesting, thanks! For ncores, we may need two allowed variables (logical or physical) or some other way to indicate which one you want. In SciPy we use this (here):

        if args.parallel is None:
            # Use number of physical cores rather than ninja's default of 2N+2,
            # to avoid out of memory issues (see gh-17941 and gh-18443)
            n_cores = cpu_count(only_physical_cores=True)
            cmd += [f"-j{n_cores}"]
        else:
            cmd += ["-j", str(args.parallel)]
dnicolodi commented 9 months ago

It seems that SciPy uses only the physical cores, thus I'll be temped to implement only that for now, and implement logical core detection only if someone asks for it. WDYT?

rgommers commented 9 months ago

Yes, I think that sounds good. It'd be good to get the feedback/reasons in case someone needs more than physical cores.

To plug in the function, I think taking the cpu_count function from SciPy unmodified would be useful, since it's pretty well-tested code by now (also through scikit-learn/loky).

dnicolodi commented 9 months ago

Can we do that from a licensing point of view? meson-python is released under the MIT license, loky and SciPy are licensed under the BSD-3 license.

rgommers commented 9 months ago

Those licenses are compatible, so there's no problem there. Technically yes, we'd have to carry over the license text and mark that part of the code as having a BSD-3 license. If it's only a few lines of code you typically wouldn't bother, but here it's >200 LoC. So probably best done in a separate file with its own SPDX header and a copy of the full license text of Loky. (I'm the author of any modifications in SciPy, so I can relicense those for use as MIT in meson-python).

dnicolodi commented 9 months ago

I was thinking to do just that, I just wanted to make sure that it is ok to include some BSD code in meson-python.

rgommers commented 9 months ago

Great. And yes, should be fine.

dnicolodi commented 9 months ago

Do you have a preference for %(ncores)d vs {ncores} vs ${ncores} namely old-style % string formatting, vs new-style string formatting, vs template strings?

The only practical difference I can see is that the first allows to specify a type for type conversion so %(ncores * 2 / 3)d is guaranteed to be the string representation of an integer, while {ncores * 2 / 3 :d} gives an error. Template strings look best IMHO, but they cannot specify the conversion type, thus ${ncores * 2 / 3} would result in the string representation of a float, which for solving the problem at hand would not work. However, we could implement the // integer division operator too, which would solve the problem.

New-style formatting is more expressive and would allow things like accessing attributes of the defined variables (not that it makes sense for integer variables as we are considering here) but it would be useful if we reuse the same mechanism for #29. On the other hand, with the Python AST based approach, we can implement that easily on top of the % string formatting syntax or template strings.

I'm leaning toward template strings and adding the // operator.

rgommers commented 9 months ago

Template strings sounds good to me, or otherwise new-style formatting. %s feels a little outdated.

// should work, but perhaps it's more practical to just state that whatever the final number is, we apply either round() or int() to it?

dnicolodi commented 9 months ago

// should work, but perhaps it's more practical to just state that whatever the final number is, we apply either round() or int() to it?

It depend on whether we are designing this as a generic facility or something that will only work with integer variables. I think it is worth to keep it generic (I think that the solution for #29 could be based on a similar mechanism, for example) thus forcing all variables to be integers does not seem right. Having a way to decide which variables should be coerced to int and which should not seems unnecessarily complicated.

rgommers commented 9 months ago

Okay, that seems like a good reason - I'm happy with // too.