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

Add WASM version (noarch, no LinAlg solvers) of bw2calc #616

Closed
wants to merge 14 commits into from
Closed

Add WASM version (noarch, no LinAlg solvers) of bw2calc #616

wants to merge 14 commits into from

Conversation

michaelweinold
Copy link
Contributor

This was discussed with @martinRenou in conda-forge/bw2calc-feedstock#14

@michaelweinold michaelweinold marked this pull request as ready for review August 23, 2023 09:47
@michaelweinold
Copy link
Contributor Author

@martinRenou, could you have a look please?

@martinRenou
Copy link
Contributor

Thanks! Would you be able to add a test (just an import should be fine), similar to what we do for the pandas recipe for example?

@michaelweinold
Copy link
Contributor Author

Hmm... micromamba seems to have no trouble installing bw2calc:

                                           __
          __  ______ ___  ____ _____ ___  / /_  ____ _
         / / / / __ `__ \/ __ `/ __ `__ \/ __ \/ __ `/
        / /_/ / / / / / / /_/ / / / / / / /_/ / /_/ /
       / .___/_/ /_/ /_/\__,_/_/ /_/ /_/_.___/\__,_/
      /_/

https://repo.mamba.pm/emscripten-forge/noarch               Using cache
conda-forge/noarch                                          Using cache

Transaction

  Prefix: /tmp/tmp9ujvahmv/prefix

  Updating specs:

   - python
   - pyjs[version='>=1.0.0,<2.0.0']
   - pytest==7.1.1
   - numpy
   - bw2calc


  Package              Version  Build              Channel                                                                Size
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  Install:
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

  + appdirs              1.4.4  pyh9f0ad1d_0       conda-forge/noarch                                                   Cached
  + atomicwrites         1.4.1  pyhd8ed1ab_0       conda-forge/noarch                                                     12kB
  + attrs               23.1.0  pyh71513ae_1       conda-forge/noarch                                                   Cached
  + bw2calc          2.0.dev13  py310h8bed8af_0    /home/runner/micromamba-root/envs/ci-env/conda-bld/emscripten-32        4kB

But the simple import test fails:

=================================== FAILURES ===================================
_____________________________ test_import_bw2calc ______________________________

    def test_import_bw2calc():
>       import bw2calc
E       ModuleNotFoundError: No module named 'bw2calc'

test_import_bw2calc.py:2: ModuleNotFoundError
=========================== short test summary info ============================
FAILED test_import_bw2calc.py::test_import_bw2calc - ModuleNotFoundError: No ...
============================== 1 failed in 0.21s ===============================
EXCEPTION pytest failed with return code: 1
Test  backend : browser-main FAILED return_code != 0
================================================================================
Test backend: browser-worker
================================================================================

@michaelweinold
Copy link
Contributor Author

I tried to add the build script we had over on conda-forge:

script: "{{ PYTHON }} -m pip install . -vv"

and then the one from the sympy recipe (for a lack of better ideas)

"{{ PYTHON }} -m pip install . --no-deps --ignore-installed --no-cache-dir -vvv"

unfortunately, that's not working.

@martinRenou
Copy link
Contributor

The relevant error message is here:

        return tuple(
            as_integer(v)
            for v in importlib.metadata.version("bw_processing")
>           .strip()
            .split(".")
        )
E       AttributeError: 'NoneType' object has no attribute 'strip'

It looks like importlib is failing to parse the version of bw_processing for some reason

@michaelweinold
Copy link
Contributor Author

Yes, so it seems - unfortunately, I don't have the resources for more troubleshooting here. Many thanks for your help!

I believe it will be best to close this PR. We'll stick with conda-forge for the Brightway packages.

@martinRenou
Copy link
Contributor

Whether it's on conda-forge as noarch or published on emscripten-forge, I suspect you will be hit by this error.

But it's better to have it as noarch on conda-forge anyway IMO.

@michaelweinold
Copy link
Contributor Author

As far as I can see, we have a basic import test in the recipe on conda-forge, which did not cause issues with bw_processing before.

@martinRenou
Copy link
Contributor

The test here runs in the browser context (similar to the context jupyterlite-xeus-python runs into) with packages coming from emscripten-forge. My point is that if it fails now with this error, if will fail later in the jupyterlite-xeus-python context with your noarch package on conda-forge.

So it's something that will need to be fixed in order for bw2calc to work in jlite.

@michaelweinold
Copy link
Contributor Author

Ah, well that's not ideal 😕

@michaelweinold
Copy link
Contributor Author

Is there a way for me to use my own fork of the bw_processing library in the build process to test a few things?

@DerThorsten
Copy link
Contributor

so lets just pach this line here https://github.com/brightway-lca/bw_processing/blob/main/bw_processing/__init__.py#L48C15-L48C32 and maybe even hard code the version.

Probably https://github.com/brightway-lca/bw_processing/blob/main/bw_processing/utils.py#L25 is not working well with pyjs / empack. Probably we filter out the metadata when packaging the pkg with empack.

@DerThorsten
Copy link
Contributor

Is there a way for me to use my own fork of the bw_processing library in the build process to test a few things?

jeah in the recipe in the source section you can use a git_url (like here https://docs.conda.io/projects/conda-build/en/stable/resources/define-metadata.html#source-section)

@martinRenou
Copy link
Contributor

Probably we filter out the metadata when packaging the pkg with empack

Yeah that's what I was thinking. I'd personally prefer patching empack to not filter out the metadata for this bw_processing package instead of patching the code (patching the code would require bumping it whenever an update comes).

@michaelweinold
Copy link
Contributor Author

@DerThorsten, if you're positive that the importlib.metadata.version("bw_processing") line is the issue, we can simply move to a better way of dynamically providing version information in the package (as suggested in this article of the Python Packaging User Guide).

@DerThorsten
Copy link
Contributor

@DerThorsten, if you're positive that the importlib.metadata.version("bw_processing") line is the issue, we can simply move to a better way of dynamically providing version information in the package (as suggested in this article of the Python Packaging User Guide).

Jeah I think so!

@michaelweinold
Copy link
Contributor Author

This might be as simple as a dash/underscore (bw-processing vs bw_processing) issue in importlib.metadata.version("bw_processing") - we're checking now.

In the recipe in the source section you can use a git_url

But how do I specify that only a single dependendency of the package in the recipe is to be loaded from git (and from a specific branch, no less)? From the conda documentation you linked, I guess that I'll need to use Source from multiple sources:

source:
  - url: https://package1.com/a.tar.bz2
    folder: stuff
  - url: https://package1.com/b.tar.bz2
    folder: stuff
  - git_url: https://github.com/conda/conda-build
    folder: conda-build

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

Successfully merging this pull request may close these issues.

None yet

3 participants