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

Cleanup Sheer Amount of Options To Set Nim Compile Switches #60

Closed
4 tasks
Pebaz opened this issue Jan 31, 2022 · 17 comments
Closed
4 tasks

Cleanup Sheer Amount of Options To Set Nim Compile Switches #60

Pebaz opened this issue Jan 31, 2022 · 17 comments
Assignees

Comments

@Pebaz
Copy link
Owner

Pebaz commented Jan 31, 2022

This would represent a breaking change and would require a major version bump.

One of the main things that needs to happen is to have hopefully 1 place to set Nim compile args. This should probably be in the form of directly setting nimporter.NimCompiler.NIM_CLI_ARGS but any better ideas are definitely appreciated.

Here are the ways that compile options can be configured:

  • Directly setting nimporter.NimCompiler.NIM_CLI_ARGS
  • Using *.nim.cfg
  • Using *.nims
  • Using switches.py (deprecated, needs to be officially removed)
  • Using the danger=True flag in build_nim_extensions()

These use cases need to be kept in mind with any new designs:

  • Local import during development
  • Precompiling for Docker
  • Source distributions that require Nim
  • Binary distributions that only require Nimporter

Some random todo items would also be nice during this major update:

  • Update test coverage numbers
  • Remove switches.py support
  • Add clear documentation on the difference and use cases of an extension library and an extension module
  • Refactor the codebase to be a bit flatter (bunch of static classes are acting basically like namespaces, module namespaces could be used instead)
@SekouDiaoNlp
Copy link
Collaborator

Hi @Pebaz how are you?

I definitely agree that we should simplify how the compilation tags are handled.

If you have some vague idea about how to better it, I will be more than welcome to implement it.

Oh and happy new year 🎈🎈🎈

@SekouDiaoNlp
Copy link
Collaborator

Removing switches.py should be fairly quick

@SekouDiaoNlp
Copy link
Collaborator

Here are the ways that compile options can be configured:

  • Directly setting nimporter.NimCompiler.NIM_CLI_ARGS

I don't think users should directly edit this file.

  • Using *.nim.cfg
  • Using *.nims

I think it should be the default behaviour.

  • Using switches.py (deprecated, needs to be officially removed)

Removing it from the next release just involves removing the warnings.

  • Using the danger=True flag in build_nim_extensions()

I think the better option would be to pass --danger true or something to that effect at the command line invocation.

These use cases need to be kept in mind with any new designs:

  • Local import during development
  • Precompiling for Docker
  • Source distributions that require Nim
  • Binary distributions that only require Nimporter

Some random todo items would also be nice during this major update:

  • Update test coverage numbers
  • Remove switches.py support

@SekouDiaoNlp
Copy link
Collaborator

Some random todo items would also be nice during this major update:

  • Update test coverage numbers
  • Remove switches.py support

I can spend some time this weekend on that.

@Pebaz
Copy link
Owner Author

Pebaz commented Feb 2, 2022

Hi @SekouDiaoNlp! Happy New Year to you as well! I'm doing great! Just enjoying the new year as things start to pick back up after the holidays. You?

I don't think users should directly edit this file.

I agree. I mentioned that as an option as it came up in this issue (#59) and it seemed to help them out I think.

I have given the cleanup some thought and I don't think we can cleanup the switches without restricting what users can do. Personally, I think that is actually a good thing because Nimporter was actually designed from a Python programmer's perspective (new to Nim) so that they wouldn't have to enter all the command line switches and whatnot.

However, most of the Nim users are pretty handy with CLI switches since they come from the C world so advanced use cases should definitely be supported.

My thoughts:

  • Only allow users to customize CLI switches using $extensionDir/nim.cfg alone and force this by passing --skipCfg, --skipUserCfg, --skipParentCfg to Nim for all compilations. I know this is an extreme solution but check this out: 😁
  • If that were to be implemented, Nimporter could rely on the fact that there is only one file to look for during compilation of a Nim extension folder. For single Nim extension modules, I'll touch on that in a bit.
  • Since Nimporter knows to look for $extensionDir/nim.cfg, we can detect if it is present and use it. However, if not, we generate it using NIM_CLI_ARGS!
  • However, NIM_CLI_ARGS needs to be renamed and some other stuff but I'll talk about that later.
  • The absolute magic 🪄 of this approach is this: whether users set NIM_CLI_ARGS per import <ext single module> or manually create an $extensionDir/nim.cfg, there will always be a nim.cfg to use since Nimporter will generate it if it doesn't exist.
  • I know there are some downsides. Namely, less freedom for users to implement their own compilation options. However, there's a problem: Nimporter wasn't designed for total freedom. It was designed so that Python programmers who don't know anything about C programming can just import the_ext and have it work seamlessly across all platforms.
  • Now for the NIM_CLI_ARGS bit. I think to easily adjust compilation options for single modules (for which Nimporter will directly ignore any *.cfgs for using --skipProjCfg, there should probably be a way to just do something like:
import nimporter

import single_module  # No customization (uses default CLI flags (probably DEFAULT_CLI_ARGS))
other_module = nimporter.nimport(
    "other_module",
    [
        "-d:release",
        "-d:danger",
        "--opt:speed",
        "..."
    ],
    # Optional:
    # ignore_cache=True
)

In summary:

  1. Disallow all config types except $extensionDir/nim.cfg for extension folders and nimport() for single modules and folders by generating a nim.cfg when needed (for single modules, just manually splat in all DEFAULT_CLI_ARGS to the CLI)
  2. Only use default args when there is no $extensionDir/nim.cfg and nimport() wasn't used (it can be passed the list of default args internally in this case)

Definitely let me know what you think and we can revise this to something better! 🙂

@Pebaz
Copy link
Owner Author

Pebaz commented Feb 2, 2022

Furthermore, I here's one more idea that might simplify the code a bit:

For extension modules (single Nim files alongside Python code), generate a temporary folder for them and put them inside, then generate the $newTempDir/nim.cfg file so that both extension modules and extension folders use the same method of compilation and discovery.

What do you think?

@SekouDiaoNlp
Copy link
Collaborator

Furthermore, I here's one more idea that might simplify the code a bit:

For extension modules (single Nim files alongside Python code), generate a temporary folder for them and put them inside, then generate the $newTempDir/nim.cfg file so that both extension modules and extension folders use the same method of compilation and discovery.

What do you think?

Definitely a great idea

@SekouDiaoNlp
Copy link
Collaborator

Hi @SekouDiaoNlp! Happy New Year to you as well! I'm doing great! Just enjoying the new year as things start to pick back up after the holidays. You?

I don't think users should directly edit this file.

I agree. I mentioned that as an option as it came up in this issue (#59) and it seemed to help them out I think.

I have given the cleanup some thought and I don't think we can cleanup the switches without restricting what users can do. Personally, I think that is actually a good thing because Nimporter was actually designed from a Python programmer's perspective (new to Nim) so that they wouldn't have to enter all the command line switches and whatnot.

However, most of the Nim users are pretty handy with CLI switches since they come from the C world so advanced use cases should definitely be supported.

My thoughts:

  • Only allow users to customize CLI switches using $extensionDir/nim.cfg alone and force this by passing --skipCfg, --skipUserCfg, --skipParentCfg to Nim for all compilations. I know this is an extreme solution but check this out: 😁
  • If that were to be implemented, Nimporter could rely on the fact that there is only one file to look for during compilation of a Nim extension folder. For single Nim extension modules, I'll touch on that in a bit.
  • Since Nimporter knows to look for $extensionDir/nim.cfg, we can detect if it is present and use it. However, if not, we generate it using NIM_CLI_ARGS!
  • However, NIM_CLI_ARGS needs to be renamed and some other stuff but I'll talk about that later.
  • The absolute magic 🪄 of this approach is this: whether users set NIM_CLI_ARGS per import <ext single module> or manually create an $extensionDir/nim.cfg, there will always be a nim.cfg to use since Nimporter will generate it if it doesn't exist.
  • I know there are some downsides. Namely, less freedom for users to implement their own compilation options. However, there's a problem: Nimporter wasn't designed for total freedom. It was designed so that Python programmers who don't know anything about C programming can just import the_ext and have it work seamlessly across all platforms.
  • Now for the NIM_CLI_ARGS bit. I think to easily adjust compilation options for single modules (for which Nimporter will directly ignore any *.cfgs for using --skipProjCfg, there should probably be a way to just do something like:
import nimporter

import single_module  # No customization (uses default CLI flags (probably DEFAULT_CLI_ARGS))
other_module = nimporter.nimport(
    "other_module",
    [
        "-d:release",
        "-d:danger",
        "--opt:speed",
        "..."
    ],
    # Optional:
    # ignore_cache=True
)

In summary:

  1. Disallow all config types except $extensionDir/nim.cfg for extension folders and nimport() for single modules and folders by generating a nim.cfg when needed (for single modules, just manually splat in all DEFAULT_CLI_ARGS to the CLI)
  2. Only use default args when there is no $extensionDir/nim.cfg and nimport() wasn't used (it can be passed the list of default args internally in this case)

Definitely let me know what you think and we can revise this to something better! 🙂

I think you have thought about it a lot.

I will see what I can whip out during the weekend.

Let me know if you have more suggestions.

@SekouDiaoNlp
Copy link
Collaborator

Furthermore, I here's one more idea that might simplify the code a bit:

For extension modules (single Nim files alongside Python code), generate a temporary folder for them and put them inside, then generate the $newTempDir/nim.cfg file so that both extension modules and extension folders use the same method of compilation and discovery.

What do you think?

This is definitely a great idea and might greatly simplify the workflow!

@SekouDiaoNlp SekouDiaoNlp self-assigned this Mar 1, 2022
@SekouDiaoNlp
Copy link
Collaborator

Hi @Pebaz how are you,

I have been playing lately with multiple implementations of removing switches and think for Python users it is the right decision.

Also always generated a $extensionDir/nim.cfg systematically will make the process straightforward

@Pebaz
Copy link
Owner Author

Pebaz commented Mar 1, 2022

Hi @SekouDiaoNlp, I'm doing great!

I agree, I also have been experimenting on another branch and am working towards what I believe to be Nimporter 2.0.0, but as of yet I haven't gotten it to work correctly. It will most likely take me a while yet to get it up and running as my availability is really in flux right now but if it works, it will be lots of breaking changes to users (hence 2.0.0 version bump).

However, we still need a deprecation path for existing users. If you can refactor Nimporter to not use switches on the current 1.x version, I'll continue to work toward trying to get this experimental Nimporter version to work. We can then release your refactor with the deprecated switches removed and then work towards ironing out whatever needs to be fixed on the Nimporter 2.0.0 experiment (if it is still worth keeping by then lol).

@SekouDiaoNlp
Copy link
Collaborator

That sounds like a good plan!

I will remove switches.py and streamline the specification of flags while keeping the structure mostly the same.

@Pebaz
Copy link
Owner Author

Pebaz commented Mar 2, 2022

Fantastic, I'll keep this thread updated as well!

@SekouDiaoNlp
Copy link
Collaborator

SekouDiaoNlp commented Mar 2, 2022

Great, I should have free time Thursday evening and the whole weekend starting Wednesday.

I will handle removing switches.py and streamline the rest.

Enjoy your day.

@Pebaz
Copy link
Owner Author

Pebaz commented Mar 22, 2022

@SekouDiaoNlp Just an update on my progress with the Nimporter 2.0.0 experiment. I have verified it to work! Now all I need to do is finish testing and polish it off! I'm hoping to have a release candidate for you to test soon (as to what "soon" means in this context I have no idea 😆).

How are things going with removing the switches? From what I found out about the Nimporter 2.0.0 design, a lot of the headache around switches largely will go away. I can go into more details once I've got more to show lol but essentially if you want, we might be able to just keep the switches in there and just migrate users onto Nimporter 2.0.0. I'm not actually sure which one would be preferable to Nimporter's user base: incremental adoption or just touch the code once. 🤷

@Pebaz
Copy link
Owner Author

Pebaz commented May 9, 2022

@SekouDiaoNlp Have you had a chance to take a look at the Release Candidate for Nimporter 2.0.0?

@Pebaz
Copy link
Owner Author

Pebaz commented Jul 27, 2023

All items have been addressed in work towards v2.0.0.

@Pebaz Pebaz closed this as completed Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants