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

fix #57 test requirements and CI tests pull brightway2 #59

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

tngTUDOR
Copy link
Contributor

merging this might need a version bump (not included in the commit).

@tngTUDOR tngTUDOR requested a review from cmutel June 27, 2022 17:07
@tngTUDOR tngTUDOR changed the title Issue 57 fix Issue #57 Jun 27, 2022
@tngTUDOR
Copy link
Contributor Author

this is in reference to issue #57

@tngTUDOR
Copy link
Contributor Author

If this pipeline definition with azure succeeds, it could also be tested in relation to bw2calc issue 55

@cmutel
Copy link
Member

cmutel commented Jun 27, 2022

This is amazing! I have been unable to understand why the pip tests weren't working, and of course it is a dumb, simple error.

However, I removed bw2calc from setup.py and requirements.txt on purpose - bw2calc should be usable without the rest of the ecosystem. Can you update the PR, it will be easier than me having to do it :)

@tngTUDOR
Copy link
Contributor Author

This is amazing! I have been unable to understand why the pip tests weren't working, and of course it is a dumb, simple error.

However, I removed bw2calc from setup.py and requirements.txt on purpose - bw2calc should be usable without the rest of the ecosystem. Can you update the PR, it will be easier than me having to do it :)

Did you mean bw2data ?

I'll work this out.

@tngTUDOR tngTUDOR changed the title fix Issue #57 fix #57 test requirements and CI tests pull brightway2 Jun 27, 2022
@tngTUDOR
Copy link
Contributor Author

tngTUDOR commented Jun 27, 2022

removing bw2data from setup.py breaks the package:
with:

...
install_requires=[
"bw_processing",
"matrix_utils",
"numpy",
"pandas",
"scipy",
"stats_arrays",
],
...
>>> import bw2calc as bc
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/code/bw2calc/__init__.py", line 55, in <module>
from .multi_lca import MultiLCA
File "/code/bw2calc/multi_lca.py", line 2, in <module>
from bw2data import calculation_setups
ModuleNotFoundError: No module named 'bw2data'

I'll revert the changes to setup.py and requirements and create a separate issue for this. -> issue #60

@tngTUDOR
Copy link
Contributor Author

The solution for the pipelines to pass was to use a virtualenv for the ubuntu-20.04-pip based testing env.

bw2data and matrix_utils are necessary, not the whole brightway2[5] package.
Previous pipeline setup was installing brightway2, hence bringing all
the deps of the brightway2 metapackage, but the master branch of the
bw2cal repo is now for brightway25, so we shouldn't bring old deps, and
we should only bring the necessary ones (not bw2io for example)

This commit also adds a pip virtual environment for the failning
ubuntu-pip setup of the testing

👷 test code being checked-out by installing first

By installing the code from source, we test the current master branch, or the code offered in a PR.

⬇️ package has dependency on bw2data not brightway2

Using bw2data as dependency keeps a coherent set of deps.
Testing dependencies are only about third party libraries required for testing, and are different
to *install_requires*.
@tngTUDOR
Copy link
Contributor Author

@cmutel , I did not add a version bmp to the PR, and I decided to keep the bw2data dependency because otherwise, the package would be broken (see comment above).

#60 has the issue explaining why we cannot remove bw2data from deps (MultiLCA needs it)

I think we can merge this, without releasing, then update MultiLCA to not depend on bw2data [if at all possible] and then do the bump.

@cmutel cmutel merged commit dfa7391 into brightway-lca:master Jun 28, 2022
@tngTUDOR tngTUDOR deleted the issue-57 branch June 28, 2022 07:09
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.

2 participants