Skip to content
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

ENH: implement variables substitution in configuration #488

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnicolodi
Copy link
Member

No description provided.

@dnicolodi dnicolodi force-pushed the substitutions branch 2 times, most recently from da08d99 to a51d22b Compare September 6, 2023 14:14
@dnicolodi dnicolodi marked this pull request as draft September 6, 2023 14:14
@dnicolodi dnicolodi force-pushed the substitutions branch 10 times, most recently from 7a1e526 to 5fcf9e1 Compare September 6, 2023 19:40
@dnicolodi
Copy link
Member Author

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

@dnicolodi dnicolodi force-pushed the substitutions branch 2 times, most recently from e12bd1f to 58bba11 Compare September 6, 2023 19:56
@dnicolodi dnicolodi added the enhancement New feature or request label Sep 10, 2023
@rgommers
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

Great. And yes, should be fine.

@dnicolodi
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

// 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
Copy link
Contributor

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

@dnicolodi dnicolodi force-pushed the substitutions branch 2 times, most recently from 000ab53 to 5c6ea12 Compare September 12, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants