Suggestion: make it easier to turn on dangerous mode (or support nims)

Benjamin-Lee commented 4 years ago

Right now, Nimporter offers great low level control over the compiler via switches.py. However, as a developer, I don't want to be worrying about maintaining my own copy of switches.py for changing simple settings.

Suppose, after #24, you decide that using the default GC is better. Now, in order to get that update, I can't just update Nimporter; I have to modify my copy-pasted-with-one-added-line switches.py file. For the more standard compiler options (danger, checks, opt), I would suggest reading from a config file or taking them as args for build_nim_extensions:

ext_modules=nimporter.build_nim_extensions(danger=True, floatChecks=True),

Or something like a .nims file:

--opt:size
--define:foo
--forceBuild

Given that the one of the major use cases for Nimporter is moving computationally intensive tasks to Nim, I imagine many users will want to use -d:danger to speed up their pipelines. For me, it makes a 3x difference, so I definitely want to use it.

juancarlospaco commented 4 years ago

I was about to ask whats the idea of switches.py ?, module.nim.cfg seems to be working at least on my local. :thinking:

Pebaz commented 4 years ago

Hello @Benjamin-Lee !

I am a little confused as to the benefits of this approach.

If this line of code worked:

ext_modules=nimporter.build_nim_extensions(danger=True, floatChecks=True),

Then even if you updated Nimporter, the settings you provide on the above line would override the ones obtained from updating Nimporter, which (from what I can tell) is the same difference?

Right now, the decision to use switches.py was largely due to having the ability to execute any Python logic to obtain the relevant switches per target platform.

I understand that .nims is a NimScript file that is a scripting language in and of itself but since Nimporter is targeted at Python users wanting to fuse Nim code into their project, I wanted to provide a Python-focused way of doing so.

Furthermore, if module.nim.cfg works (I have not had the opportunity to test yet), then it looks like both methods work?

Let me know your thoughts on this! 🙂

juancarlospaco commented 4 years ago

I never used switches.py, the *.nim.cfg seems to work. If *.nim.cfg confirmed to work, I think that switches.py should be deprecated, too many duplication already, DRY.

I used it on my package, it builds ok, ships the files ok, but when installing, it does not copy the nim files on the disk, I checked it, even if it suppose to, silently skips them, users reported the problem, feel free to try installing yourself.

I know is more because of Python toolchain for installing stuff kinda sux, but yeah it wont work. :shrug:

Benjamin-Lee commented 4 years ago

I am a little confused as to the benefits of this approach.

If this line of code worked:
ext_modules=nimporter.build_nim_extensions(danger=True, floatChecks=True),
Then even if you updated Nimporter, the settings you provide on the above line would override the ones obtained from updating Nimporter, which (from what I can tell) is the same difference?

Sorry for the confusion, allow me to clarify. Yes, the keyword args would override the defaults. However, for whatever keyword args are not specified by the user, updates in future versions of Nimporter could be reflected. Going back to the #24 example, suppose that the current default is

def build_nim_extensions(gc=“markAndSweep”, danger=False, ...):

Then, in my library, I can do this since I only care about turning off checks:

ext_modules=nimporter.build_nim_extensions(danger=True)

Now, if you were to change the default GC, when I update Nimporter, I would get the new GC without having to maintain my own switched.py file.

In essence, switches.py is an all or nothing solution. You either use the default Nimporter compile args or have to write your own file with platform detection and all compile args specified. I’m hoping to create an intermediate between the two extremes, in which I can outsource the management of the compile flags to Nimporter except with an easy way to adjust the basics (danger, gc, etc).

If you’d like, I can try to make a prototype as a PR.

Pebaz commented 4 years ago

I never used switches.py, the *.nim.cfg seems to work. If *.nim.cfg confirmed to work, I think that switches.py should be deprecated, too many duplication already, DRY.

I used it on my package, it builds ok, ships the files ok, but when installing, it does not copy the nim files on the disk, I checked it, even if it suppose to, silently skips them, users reported the problem, feel free to try installing yourself.

I know is more because of Python toolchain for installing stuff kinda sux, but yeah it wont work. 🤷

Hello @juancarlospaco !

I have taken a look at your (awesome!) library, but I was unable to find any use of Nimporter within it. As far as I am able to tell, you have a custom build setup in setup.py that uses different folders containing C code that get compiled per-platform. I may have completely missed it though 😅

Nimporter could really help with this distribution scenario. To use Nimporter effectively, you will need to do these things:

Have a top-level faster_than_requests.py file that imports Nimporter and then imports your faster_than_requests.nim file.
In your setup file, use nimporter.build_nim_extensions() to find the faster_than_requests.nim file.
It may be a good idea to move your Nim file outside of the src dir or Nimporter will register src as a package and require you to import src.faster_than_requests most likely.
Distribute the binary wheels per platform as source distributions are only supported for bundling actual Nim source files (not C code).

Let me know if I can clarify anything! 🌴

Pebaz commented 4 years ago

I am a little confused as to the benefits of this approach. If this line of code worked:
ext_modules=nimporter.build_nim_extensions(danger=True, floatChecks=True),
Then even if you updated Nimporter, the settings you provide on the above line would override the ones obtained from updating Nimporter, which (from what I can tell) is the same difference?
Sorry for the confusion, allow me to clarify. Yes, the keyword args would override the defaults. However, for whatever keyword args are not specified by the user, updates in future versions of Nimporter could be reflected. Going back to the #24 example, suppose that the current default is
def build_nim_extensions(gc=“markAndSweep”, danger=False, ...):
Then, in my library, I can do this since I only care about turning off checks:
ext_modules=nimporter.build_nim_extensions(danger=True)
Now, if you were to change the default GC, when I update Nimporter, I would get the new GC without having to maintain my own switched.py file.

In essence, switches.py is an all or nothing solution. You either use the default Nimporter compile args or have to write your own file with platform detection and all compile args specified. I’m hoping to create an intermediate between the two extremes, in which I can outsource the management of the compile flags to Nimporter except with an easy way to adjust the basics (danger, gc, etc).

If you’d like, I can try to make a prototype as a PR.

Hello @Benjamin-Lee !

Thank you for clarifying this! As this would require a semi-involved change to Nimporter, I will need more time to look into how I want to go about this.

juancarlospaco commented 4 years ago

You can download from PYPI from here and try to install it locally on your PC: https://pypi.org/project/faster-than-requests/0.9.9/#files This is the code: https://github.com/juancarlospaco/faster-than-requests/tree/467f7c261d410ca642d6d398bd2a8f8c20981ab5

Pebaz commented 4 years ago

Ah yes, thank you @juancarlospaco I see it now.

I have inspected your code and I see no Python file that imports Nimporter and then imports your Nim file faster_than_requests.nim.

Alternatively, you may also specify ext_modules=nimporter.build_nim_extensions() in your setup.py or the equivalent command in setup.cfg to have Nimporter find and bundle your faster_than_requests.nim file.

juancarlospaco commented 4 years ago

But I wanted source distributions, not binary.

I dont find the correct structure on the documentation, maybe a function that generates a project template like nimble init but for Nimporter. :thinking:

Benjamin-Lee commented 4 years ago

@Pebaz sorry to bump this back up, but we're working in the release of librtd so I'd like to make it so that it runs with -d:danger. Short of a flag (or similar) within Nimporter, would you be willing to add instructions on how to activate it? Switches.py seems to be somewhat under documented.

Pebaz commented 4 years ago

Hello @Benjamin-Lee !

I'm sorry for the delay, I have been onboarding to a new job for the past month and a half and have been pretty focused on that.

However, I have implemented the requested change to add a danger=True flag to the build_nim_extensions function.

Can you pull the most recent code from the master branch via:

$ pip install git+https://github.com/Pebaz/nimporter

And then test this new flag?

from setuptools import setup
import nimporter

setup(
    ...
    ext_modules=nimporter.build_nim_extensions(danger=True)
)

I have run this on the testing machines and they all pass. Let me know if this doesn't work for you and I will fix it.

Also, whenever you can confirm this works, I will update the documentation with how to use it.

Thank you for pinging me about this and I hope this works out! 😀

Benjamin-Lee commented 4 years ago

Just tested it and got the speedup I was expecting (about 20%)!

~/D/P/R/l/librtd-py ❯❯❯ librtd 3 ../../coronavirus_genome_phylogeny/data/non_degenerate_complete_genomes.fasta testymctestyface.fasta
Info: Using librtd v0.1 by Benjamin D. Lee. (c) 2020 IQT Labs, LLC.
[=====================================================================================================================================] 100.00%
Success: Analyzed RTD for 3780 sequences totaling 110383091 bp in 26.6 seconds.
~/D/P/R/l/librtd-py ❯❯❯ cd ../librtd; nimble install -y; cd ../librtd-py/; nimporter clean; pip install -e .
   Warning: Package 'librtd' has an incorrect structure. It should contain a single directory hierarchy for source files, named 'librtd', but file 'tests.nim' is in a directory named 'tests' instead. This will be an error in the future.
      Hint: If 'tests' contains source files for building 'librtd', rename it to 'librtd'. Otherwise, prevent its installation by adding `skipDirs = @["tests"]` to the .nimble file.
  Verifying dependencies for librtd@0.0.1
      Info: Dependency on progress@>= 1.1.1 already satisfied
  Verifying dependencies for progress@1.1.1
 Installing librtd@0.0.1
    Prompt: librtd@0.0.1 already exists. Overwrite? -> [forced yes]
   Success: librtd installed successfully.
Cleaning Directory: /Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py
Deleted:            /Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py/MANIFEST.in
Deleted Folder:     /Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py/nim-extensions
Deleted Folder:     /Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py/build/temp.macosx-10.15-x86_64-3.7/nim-extensions
Deleted:            /Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py/librtdpy/__pycache__/librtdpy.so
Deleted:            /Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py/librtdpy/__pycache__/librtdpy.nim.hash
Obtaining file:///Users/BenjaminLee/Desktop/Python/Research/librtd/librtd-py
Requirement already satisfied: docopt in /Users/BenjaminLee/.virtualenvs/librtd/lib/python3.7/site-packages (from librtd==0.0.1) (0.6.2)
Installing collected packages: librtd
  Attempting uninstall: librtd
    Found existing installation: librtd 0.0.1
    Uninstalling librtd-0.0.1:
      Successfully uninstalled librtd-0.0.1
  Running setup.py develop for librtd
Successfully installed librtd
~/D/P/R/l/librtd-py ❯❯❯ rm testymctestyface.fasta
~/D/P/R/l/librtd-py ❯❯❯ librtd 3 ../../coronavirus_genome_phylogeny/data/non_degenerate_complete_genomes.fasta testymctestyface.fasta
/Users/BenjaminLee/.nimble/pkgs/nimpy-0.1.0/nimpy.nim(972, 32) Warning: Cannot prove that 'arg1k' is initialized. This will become a compile time error in the future. [ProveInit]
Info: Using librtd v0.1 by Benjamin D. Lee. (c) 2020 IQT Labs, LLC.
Warning: Not compiled as a dangerous release build. Recompile with -d:danger for maximum performance.
[=====================================================================================================================================] 100.00%
Success: Analyzed RTD for 3780 sequences totaling 110383091 bp in 33.7 seconds.

Benjamin-Lee commented 4 years ago

PS: Not sure what the arg1k warning is but it doesn't seem to break anything so 🤷‍♂️

Edit: issue filed in https://github.com/yglukhov/nimpy/issues/163

Pebaz commented 4 years ago

Hello @Benjamin-Lee that is fantastic, glad to hear it! I will push out a new release shortly to PyPi that you can use. Definitely let me know of any issues if anything comes up!

Pebaz commented 4 years ago

Hello @juancarlospaco !

I'm sorry for the delayed reply regarding your question of source/binary distributions.

Nimporter does not support source distributions.

It does, however, support source distributions that include Nim source files along with Python files. This means that it is not possible to create a source distribution containing Nim files compiled to C and distribute that to every platform because Nim code compiled to C is only compatible with the platform it was compiled for.

To get around this, Nimporter supports binary distributions. Simply use python setup.py bdist_wheel to generate a wheel that is compatible with the platform it was compiled from. The resulting wheel can be uploaded to Pypi for easy installation per platform.

Also, the recommended folder structure for Nimporter projects has now been documented.

I will go ahead and close this issue for now but if you have any other questions definitely let me know and I will be happy to answer them!

Pebaz / nimporter

Suggestion: make it easier to turn on dangerous mode (or support nims) #25