search

fable-compiler / Fable.Python

Python bindings for Fable
https://fable.io/docs/
MIT License
135 stars 10 forks source link

Module import path calculation #49

Closed thautwarm closed 2 years ago

thautwarm commented 2 years ago

I've recently found a few cases that the generated code doesn't work due to incorrect module import.

For instance, I'm using .fsproj like below, which works in .NET build. The OutputType is not exe.

<ItemGroup>
        <Compile Include="pykg_modules\\local\\D.fs"/>
</ItemGroup>

The project structure is

- $ROOT
    - myproj.fsproj

    - pykg_modules
        - local

            - D.fs

    - outDir  # I expect this is a generated python package

          - pykg_modules(generated)
             - local (generated)

                   - d.py (generated)

          - fable_modules(generated)      
              - fable_library (generated)
                 - ...

The generated code is not working.

For instance, if I do printfn "hello world" in D.fs,

import outDir.pykg_modules.local.d in $ROOT, it fails for 

...\outDir\pykg_modules\local\a.py in <module>
      1 from typing import (TypeVar, Any)
----> 2 from ..fable_library.string import (to_console, printf)
      3
      4 __A = TypeVar("__A")
      5

 from ..fable_library.string import (to_console, printf)
      3
      4 __A = TypeVar("__A")
      5

ModuleNotFoundError: No module named 'outDir.pykg_modules.fable_library'

If I import pkg_modules.local.d in outDir, it fails for

...\outDir\pykg_modules\local\a.py in <module>
      1 from typing import (TypeVar, Any)
----> 2 from ..fable_library.string import (to_console, printf)
      3
      4 __A = TypeVar("__A")
      5

ModuleNotFoundError: No module named 'pykg_modules.fable_library'

I want to fix this and make PRs, and I've digged into this:

https://github.com/fable-compiler/Fable/blob/f6c58f14d0db24973e9b0b176b51f3f89adc0232/src/Fable.Transforms/Python/Fable2Python.fs#L499

Could you help me with the following detail information?

With such information, I can manage to make it work.

thautwarm commented 2 years ago

If we can find the generated path of com.CurrentFile, we can solve this.

dbrattli commented 2 years ago

I have a repro here now. Looking at things ...

thautwarm commented 2 years ago

Currently for any FSharp module occurred in the ProjectReference, the information of the referenced project is missing, and the import path is guessed.

Even in this case, we can still get the correct import path, if we know where the generated Python file is.. but com.CurrentFile (EDIT: sometimes) only tells the source path of the FSharp module...

dbrattli commented 2 years ago

Yes, but we know the output file will be as relative to the output dir as the currentFile is relative to the project file. This is what the current code tries to do, but it's a bit messy 😬

dbrattli commented 2 years ago

This line needs to be removed .Replace("...fable_modules.", "..") // attempted relative import beyond top-level package

But then something else will break. I'll look into what the problem was ...

thautwarm commented 2 years ago

Look at this example...

modulePath: ../fable-library-py/fable_library/Native.fs
projectFile: $ROOT/Fable/src/fable-library-py/fable_library/Fable.Library.fsproj
currentFile: $ROOT/Fable/src/fable-library/Array.fs
outputDir: Some($ROOT/Fable/build/fable-library-py/fable_library)

The output import path should be ".native", but this cannot be computed, if we don't know the information provided by another project $ROOT/Fable/src/fable-library/Fable.Library.fsproj.

This is because we don't really know output path of Array.fs. fable-library-py/array.py, or fable-library-py/some_directory/array.py? Array.fs is included in $ROOT/Fable/src/fable-library/Fable.Library.fsproj, which is invisible to us.

This is because we don't really know output path of Array.fs. Its absolute path is based on where the unknown $ROOT/Fable/src/fable-library/Fable.Library.fsproj is.

dbrattli commented 2 years ago

We know it will be output in the path relative to the outputDir where the relative path is the difference between (common prefix of currentFile and project file) and the currentFile. So it should actually be .fable_library.native for this case, but it is currently handled as a special case where build.fsx copies them all into the same dir. We might fix this in future when we have a bit more generic import handling.

Anyways, I think I have fixed this issue for now by removing the line that was the problem and made sure a module inside fable_modules cannot import out (and back in) of fable_modules (see PR)

thautwarm commented 2 years ago

I do not understand why it should befable_library.native here, asbuild/fable-library-py/fable_library/../fable-library-py/fable_library/Native.fs is not the correct path.

And I don't think we need special handling for import paths. In my opnion we just need to compile F# sources into .py, and then move them into outDir, just like we move the project directory (only .py parts) into outDir. For external packages, we put them inside the outDir.

Then all these paths are statically known, next, we do import path resolution, which is pretty easy without any hack. No corner case will happen. fable_library can import itself, external packages (no matter they are nuget ones or others) can import fable_library and each other.

Besides, we'd better not to specially treat OutputType=EXE. Always generating outDir as a python package will simplify a lot of works. If we need a script, the best option is to directly invoke the python package (such approach is widely adopted, like in PyCharm, VSCode Python extension, poetry and setuptools entry_points).

thautwarm commented 2 years ago

Anyway... could you please make a release of such patch? I know you are busy, but I badly need this and get stuck by this issue since 2 days ago. Sorry to bother you!

dbrattli commented 2 years ago

https://www.nuget.org/packages/fable-py/4.0.0-alpha-029

PS: Note that because of my job I can be more or less unresponsive during weekdays:

thautwarm commented 2 years ago

Thank you!

dbrattli commented 2 years ago

Feel free to improve the import handling. But I'm not ready to remove support scripts and absolute import handling just yet. Ref .eg timeflies demo. It should not be a package imo. But it could always be handled by a separate function / logic. The best way would be if we could mock all use-cases within the test project so we can test things to know that things works as expected. I'll put it on the TODO list.

dbrattli commented 2 years ago

Closing this since I believe it's been fixed. Fable Python may operate in two different modes. Embedded (using fable-modules) as normal, and --fableLib site-packages which will compile all dependencies as Python packages. Then you may use poetry to list the dependencies and they choose to use local path references from fable-modules or PyPI based from (env) site-packages.