qpv-research-group / solcore5

A multi-scale, python-based library for the modelling of solar cells and semiconductor materials
https://www.solcore.solar/
Other
133 stars 77 forks source link

Finalizing changes after implementing new build system #243

Closed phoebe-p closed 1 year ago

phoebe-p commented 1 year ago

Following the change to the build system merged in #242, there were some remaining action items in the PR which were not checked off before I merged it:

@Abelarm thank you very much for all your work on this, we really appreciate it and hopefully it will make Solcore a lot more user-friendly :) For these remaining points, the first and third one seem simple (setup.cfg is no longer necessary is I understand it so can just delete it, and I will update the documentation to reflect that installation with the PDD should now be simple using wheels on all platforms). For the second point, is this simply moving the contents of mypi.ini into pyproject.toml or is more work required? For the final point, perhaps this was already resolved or I am missing something - test_examples.py seems to run without problem in the scheduled tests.

Additional points:

Abelarm commented 1 year ago

Hi @phoebe-p,

Sorry for the late answer.

Going one by one:

Other topic related:

phoebe-p commented 1 year ago

See PR #245.

I would not count this as a slow reply @Abelarm, especially since this is not your job (maybe my standards are warped by academia ;))

Noted the points re: setup.cfg. isort is already in pyproject.toml and I have added added tool.mypy and tool.mypy-setup. For flake8, the options were also already in pyproject.toml, so following the link you posted, I have added the relevant section to .pre-commit-config.yaml, I think this is all? It (flake8) is working on my computer with these changes anyway, I'm not sure how to check if mypy and isort are working to be honest...

I have added the build for 3.11 in build_deploy_wheels.

I will have a look and see if I can find out what's going on with the tests on Windows.

I will squash the commits from testing the build system in this branch before merging as well (or you can do it as well, since perhaps you would write a more informative new commit message!)

phoebe-p commented 1 year ago

@all-contributors please add @Abelarm for test

allcontributors[bot] commented 1 year ago

@phoebe-p

I've put up a pull request to add @Abelarm! :tada:

Abelarm commented 1 year ago

I worked close to academia but not close enough for having my time-to-answer warped ahahahah Anyway I trust your squashing better then mine!

Happy to help with anything!

phoebe-p commented 1 year ago

Ok, the examples on Windows were getting stuck/failing due to Ngspice. I am not sure why and I don't want to spend time now checking locally (I would have to install Ngspice on my Windows computer). So for now I am just skipping the Ngspice installation in Windows so that example (Quasi3D_3J_solar_Cell.ipynb) will be skipped, it's better to have all the other examples running in Windows at least, and that one is still running in Linux and MacOS.

Since I already merged your branch into develop, I think I will leave the commit history as it is -- it's a bit messy but if I squash now then it may cause problems for anyone who happens to have cloned/pulled develop in the last two weeks (I think - to be honest I get quite confused by Git still sometimes). It is not the first set of messy commits in Solcore's history anyway, but something I will keep in mind for the future before merging!

phoebe-p commented 1 year ago

Ok, one more issue @Abelarm. I wanted to check if the instructions for installing in editable mode (pip install -e .) still work but it does not, I get this:

Obtaining file:///Users/phoebe/Documents/develop/solcore5
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
ERROR: Project file:///Users/phoebe/Documents/develop/solcore5 has a 'pyproject.toml' and its build backend is missing the 'build_editable' hook. Since it does not have a 'setup.py' nor a 'setup.cfg', it cannot be installed in editable mode. Consider using a build backend that supports PEP 660.

I tried installing the most recent version of meson-python directly from their GitHub since they recently added a build_editable hook in their development version (not yet on PyPI), so I think it should support PEP 660, but I still get the same error.

Also, does the "with_pdd" installation option still do anything? I think it is set to true by default now in meson_options.txt (so to do a local install, pip install . installs Solcore with the PDD, so pip install . --install-option="--with_pdd" is redundant)?

Abelarm commented 1 year ago

Hi @phoebe-p,

Regarding the install in editable mode, from reading the PEP517 the build part, seems like that it create a build environment from scratch (from the pyproject.toml) so for this reason even if you install it from GitHub it take the one from pypi. So probably we need to wait for the official release. I am not sure but that's what I got from reading, there should also be a way to do it not in a "isolate environment" but we can check

Regarding the option of --with_pdd it's not necessary, I added the option just in case, but the default install everything.

phoebe-p commented 1 year ago

Thank you, I hadn't thought about creating a build environment from scratch at all even though it makes sense! It turns out there is a pre-release of meson-python 0.13 on PyPI, and changing the requirement in pyproject.toml to 'meson-python==0.13.0rc0' it now works fine installing in editable mode. Thanks again for all the help with the new build system & the other issues you tackled recently!

phoebe-p commented 1 year ago

Ok, one more question from me @dalonsoa @Abelarm - is there any reason to keep the MANIFEST.in file? As far as I can tell it doesn't do anything (and it was outdated anyway), the build is fine without it, but I want to check there is not some other reason to keep it before deleting.

dalonsoa commented 1 year ago

It is, indeed, not used anymore. The purpose of MANIFEST.in was to collect all the non-python files in the repository (like the materials nk indices) and distribute them with the Solcore package. I would assume that the new wheels incorporate all of those data files - I haven't checked. If they don't, either with the MANIFEST or other way, there has to be a way of including those files.

phoebe-p commented 1 year ago

Yes, the wheel files definitely also include the non-Python files (whether they are listed in the MANIFEST or not). I will remove it, since I tested local installation and the build without it and it doesn't cause problems. Thanks!

Abelarm commented 1 year ago

Yep I remember that the non-python files were added on the meson build without the help of the manifest, I forgot to remove it :(

Anyway super happy to help!!

If you have any more ideas/improvements around python/devops open an issue and if I am able I'll try to tackle it.

phoebe-p commented 1 year ago

pyproject.txt Hi @Abelarm, sorry to reopen this old issue. While the build is now working and I haven't had any issues with people using pip install solcore, I am finding that it is now impossible to install Solcore from a local source even without the PDD if the correct compilers are not installed (this is an issue for e.g. people who are developing Solcore on Windows, who are not interested in using the PDD at all, but want to be able to install from a local version of Solcore without compiling anything, or people who want to install from a non-default branch of Solcore so can't use pip install solcore). Everything works fine if they have the relevant compilers etc. but if they are missing (or meson is not able to link the correct compiler even when it is installed), I cannot install at all, because even if I run:

python -m devpy build -- -Dwith_pdd=false -Dinstall_test=true

so the PDD won't be installed, Meson still looks for all the relevant compilers and fails if it cannot find them, because it does not know it won't need to use them. I tried to add an if/else statement to meson.build, but it seems you cannot build a pure Python package with Meson (which makes sense, since that isn't what it's for). In principle this should be very trivial of course since it's a pure Python package without the PDD (the pyproject.toml attached seems to work perfectly). However, I am not if it's possible to configure things so that the user can choose with some command line option whether to install with the PDD or skip the meson build altogether?

Abelarm commented 1 year ago

Hey @phoebe-p no problem!

First regarding the build without the compilers we need to find a way to make the meson not look for the compilers if we are not building PDD.

Could you post here the error trace so we can debug it?

Secondly if you run 'pip install .' does it work?

phoebe-p commented 1 year ago

Here is the output on Windows without any compilers installed (from running pip install . - it fails whether I run the devpy command directly or pip install . since they both end up calling meson anyway).

(solcore_testing) PS C:\Users\EQE_setup\Documents\solcore5> pip install .
WARNING: Ignoring invalid distribution -umpy (c:\programdata\anaconda3\lib\site-packages)
WARNING: Ignoring invalid distribution -umpy (c:\programdata\anaconda3\lib\site-packages)
Processing c:\users\eqe_setup\documents\solcore5
  DEPRECATION: A future pip version will change local packages to be built in-place without first copying to a temporary directory. We recommend you use --use-feature=in-tree-build to test your packages with this new behavior before it becomes the default.
   pip 21.3 will remove support for this functionality. You can find discussion regarding this at https://github.com/pypa/pip/issues/7555.
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
    Preparing wheel metadata ... error
    ERROR: Command errored out with exit status 1:
     command: 'C:\ProgramData\Anaconda3\python.exe' 'C:\ProgramData\Anaconda3\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py' prepare_metadata_for_build_wheel 'C:\Users\EQE_SE~1\AppData\Local\Temp\tmpzzezmdl7'
         cwd: C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o
    Complete output (21 lines):
    + meson setup --prefix=C:\ProgramData\Anaconda3 C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o\.mesonpy-o1u3r3g3\build --native-file=C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o\.mesonpy-native-file.ini -Ddebug=false -Doptimization=2 --python.purelibdir C:\ProgramData\Anaconda3\Lib\site-packages --python.platlibdir C:\ProgramData\Anaconda3\Lib\site-packages
    The Meson build system
    Version: 1.1.1
    Source dir: C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o
    Build dir: C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o\.mesonpy-o1u3r3g3\build
    Build type: native build
    Project name: solcore
    Project version: 5.9.1
    WARNING: Failed to activate VS environment: Could not find C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe

    ..\..\meson.build:1:0: ERROR: Unknown compiler(s): [['icl'], ['cl'], ['cc'], ['gcc'], ['clang'], ['clang-cl'], ['pgcc']]
    The following exception(s) were encountered:
    Running `icl ""` gave "[WinError 2] The system cannot find the file specified"
    Running `cl /?` gave "[WinError 2] The system cannot find the file specified"
    Running `cc --version` gave "[WinError 2] The system cannot find the file specified"
    Running `gcc --version` gave "[WinError 2] The system cannot find the file specified"
    Running `clang --version` gave "[WinError 2] The system cannot find the file specified"
    Running `clang-cl /?` gave "[WinError 2] The system cannot find the file specified"
    Running `pgcc --version` gave "[WinError 2] The system cannot find the file specified"

    A full log can be found at C:\Users\EQE_setup\AppData\Local\Temp\pip-req-build-oaqy8t8o\.mesonpy-o1u3r3g3\build\meson-logs\meson-log.txt
    ----------------------------------------
WARNING: Discarding file:///C:/Users/EQE_setup/Documents/solcore5. Command errored out with exit status 1: 'C:\ProgramData\Anaconda3\python.exe' 'C:\ProgramData\Anaconda3\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py' prepare_metadata_for_build_wheel 'C:\Users\EQE_SE~1\AppData\Local\Temp\tmpzzezmdl7' Check the logs for full command output.
ERROR: Command errored out with exit status 1: 'C:\ProgramData\Anaconda3\python.exe' 'C:\ProgramData\Anaconda3\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py' prepare_metadata_for_build_wheel 'C:\Users\EQE_SE~1\AppData\Local\Temp\tmpzzezmdl7' Check the logs for full command output.
Abelarm commented 1 year ago

Hi @phoebe-p

I modified a bit the meson's files such that you can build a "pure" only python if you run the build/install with the -Dwith_pdd=false

the full command should be: pip install . --config-setting=setup-args="-Dwith_pdd=false"

Here is the branch: https://github.com/Abelarm/solcore5/tree/pure_python_meson, maybe you can try on your windows machine.

please let me know if it works for you... it should 🙏

phoebe-p commented 1 year ago

It works! Thanks a lot :)