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 dynamic dependency in pyproject.toml #98

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Conversation

Tieqiong
Copy link
Contributor

@Tieqiong Tieqiong commented Sep 3, 2024

closes #97

@sbillinge Please check and better test it on your setup.

We probably want this in the cookiecutter too, maybe also change the worked packages.

Another thing that might worth notice is do we want to include requirements/build.txt? This is specific to diffpy.pdffit2's gsl dependency. However setuptool and python are included, which in theory should already be present if a user is installing this package to a conda environment with python installed.

@bobleesj
Copy link
Contributor

bobleesj commented Sep 3, 2024

@Tieqiong Noticed the same bug with bg-mpl-stylesheets.

I checked cookiecutter`s pyproject.toml that it doesn't have the dependency install section.

https://github.com/Billingegroup/cookiecutter/blob/main/%7B%7B%20cookiecutter.repo_name%20%7D%7D/pyproject.toml

@sbillinge sbillinge merged commit f67977d into diffpy:main Sep 3, 2024
2 checks passed
@sbillinge
Copy link
Contributor

Thanks @Tieqiong I will test

@sbillinge
Copy link
Contributor

Very slow internet here so not sure if I can test befoer I have to board plane. But in answer to the other questions, I agree that adding a requirements/build.txt in cookiecutter would be good as this will differ between c and non-c packages. Is there a way to add the c requirements but somehow commentd out? It iseasier to delete things that have to add them and not know the correct name/syntax

@sbillinge
Copy link
Contributor

OK, I tested on local and it worked. I will push out a release bump to pypi if I can (internet!)

@sbillinge
Copy link
Contributor

@Tieqiong @bobleesj please can you test by pip installing diffpy.structure==3.2.1rc1? I pushed this to pypi.

I couldn't build the pycifrw dependency on my windows computer. I get:

      copying src\YappsStarParser_STAR2.py -> build\lib.win-amd64-cpython-312\CifFile
      copying src\YappsStarParser_2_0.py -> build\lib.win-amd64-cpython-312\CifFile
      copying src\StarFile.py -> build\lib.win-amd64-cpython-312\CifFile
      copying src\TypeContentsParser.py -> build\lib.win-amd64-cpython-312\CifFile
      copying src\parsetab.py -> build\lib.win-amd64-cpython-312\CifFile
      creating build\lib.win-amd64-cpython-312\CifFile\drel
      copying src\drel\drel_ast_yacc.py -> build\lib.win-amd64-cpython-312\CifFile\drel
      copying src\drel\drel_lex.py -> build\lib.win-amd64-cpython-312\CifFile\drel
      copying src\drel\drel_runtime.py -> build\lib.win-amd64-cpython-312\CifFile\drel
      copying src\drel\parsetab.py -> build\lib.win-amd64-cpython-312\CifFile\drel
      copying src\drel\py_from_ast.py -> build\lib.win-amd64-cpython-312\CifFile\drel
      copying src\drel\__init__.py -> build\lib.win-amd64-cpython-312\CifFile\drel
      running build_ext
      building 'CifFile.StarScan' extension
      error: Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for pycifrw
Successfully built diffpy.structure
Failed to build pycifrw
ERROR: ERROR: Failed to build installable wheels for some pyproject.toml based projects (pycifrw)

(not the full trace) which is pretty annoying.

@sbillinge
Copy link
Contributor

I guess this is because pycifrw only deploys their code to pypi, as we do for pdffit2, and so a pip install of it tries to build locally and I don't have the right compiler.

@bobleesj
Copy link
Contributor

bobleesj commented Sep 3, 2024

@sbillinge

  Attempting uninstall: diffpy.structure
    Found existing installation: diffpy.structure 3.2.1rc0
    Uninstalling diffpy.structure-3.2.1rc0:
      Successfully uninstalled diffpy.structure-3.2.1rc0
Successfully installed diffpy.structure-3.2.1rc1 numpy-1.26.4 ply-3.11 pycifrw-4.4.6

It works for me. Mac M1

Testing the import

from diffpy.structure import loadStructure
print(loadStructure) #<function loadStructure at 0x10db8ac00>

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 3, 2024

@sbillinge It'll work with the Microsoft c++ compiler installed.

image

@sbillinge
Copy link
Contributor

@sbillinge

  Attempting uninstall: diffpy.structure
    Found existing installation: diffpy.structure 3.2.1rc0
    Uninstalling diffpy.structure-3.2.1rc0:
      Successfully uninstalled diffpy.structure-3.2.1rc0
Successfully installed diffpy.structure-3.2.1rc1 numpy-1.26.4 ply-3.11 pycifrw-4.4.6

It works for me. Mac M1

Testing the import

from diffpy.structure import loadStructure
print(loadStructure) #<function loadStructure at 0x10db8ac00>

I don't think this is testing what we want. You have to do an install in a new clean empty environment.

@bobleesj
Copy link
Contributor

bobleesj commented Sep 3, 2024

@sbillinge

  Attempting uninstall: diffpy.structure
    Found existing installation: diffpy.structure 3.2.1rc0
    Uninstalling diffpy.structure-3.2.1rc0:
      Successfully uninstalled diffpy.structure-3.2.1rc0
Successfully installed diffpy.structure-3.2.1rc1 numpy-1.26.4 ply-3.11 pycifrw-4.4.6

It works for me. Mac M1
Testing the import

from diffpy.structure import loadStructure
print(loadStructure) #<function loadStructure at 0x10db8ac00>

I don't think this is testing what we want. You have to do an install in a new clean empty environment.

@sbillinge Okay. pip install test passed from a new env using the recent 3.2.1 version.

Installing collected packages: ply, numpy, pycifrw, diffpy.structure
Successfully installed diffpy.structure-3.2.1 numpy-1.26.4 ply-3.11 pycifrw-4.4.6

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 4, 2024

@sbillinge I think in fact we can't include build.txt into pyproject.toml as it will give errors like the following:

image

so I guess instead of skipping python requirement from build.txt as it's already installed in the environment, pip will actually try to find a version of python that is compatible with the already-installed python=3.12, which is no go.

This is specific to python in build.txt but not setuptools (the following I remove python in build.txt):

image

@sbillinge
Copy link
Contributor

so is everything fixed if we just remove python from build.txt? This seems like a good solution?

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 4, 2024

so is everything fixed if we just remove python from build.txt? This seems like a good solution?

as far as I know yes I think this will get rid of the error. However I'm not exactly sure if we want to remove it because I have no idea why it was included in the first place...

@sbillinge
Copy link
Contributor

Yah, if we don't understand what is going on we could just be introducing more problems

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 5, 2024

@sbillinge So I searched around online and asked Andrew, it seems like there's nothing special with this python in the requirement list. Do we want to remove it and add build.txt into dynamic dependencies?

@sbillinge
Copy link
Contributor

@sbillinge So I searched around online and asked Andrew, it seems like there's nothing special with this python in the requirement list. Do we want to remove it and add build.txt into dynamic dependencies?

What do you mean by "nothing special"?

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 5, 2024

I didn't find any documentation that have it as convention, and according to Andrew:

It should be fine to remove for most packages, the old packages had them so I didn't bother taking it out

So by "nothing special" I mean there's no hidden mechanism that require it to be there

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 5, 2024

I guess a better way to go is to figure out how to build wheel so that build.txt would not be needed for pip install for sure.

@sbillinge
Copy link
Contributor

I didn't find any documentation that have it as convention, and according to Andrew:

It should be fine to remove for most packages, the old packages had them so I didn't bother taking it out

So by "nothing special" I mean there's no hidden mechanism that require it to be there

Do you mean that it doesn't have to be there?

@sbillinge
Copy link
Contributor

@Tieqiong please can you propose a solution for what you want the yml to look like? We will work on building the wheels but we also want the source bundle to be right either way.

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Sep 6, 2024

Yes it doesn't have to be there, but whether it should be there depends on how we are using the file (I think we are talking about build.txt not yml file right?).

If we use the file as a source of information during pip installation, then we need to remove python because it'll cause error for pip.
If we build and distribute wheel, then pip don't need information from it. In this case it's actually good to keep the file as it is, as the file will be solely for human to read.

@sbillinge
Copy link
Contributor

No human will read it, so let's get rid of the python in there. We want pip to be able to build from sources even if we distribute wheels

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.

Dependencies not installed when installing v3.2.0 from PyPI
3 participants