Skip to content

Conversation

Tieqiong
Copy link
Contributor

No description provided.

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Aug 27, 2024

Ok after hours of investigation, the error is only happening specifically when running pytest (not unittest, which is working fine) on linux (not other platforms) when using only regular pip install (didn't install with -e before).

the error could be removed by instead of using pip install -e ., as it will generate pdffit2.cpython-312-x86_64-linux-gnu.so file which I guess is a compiled C extension inside the tests directory, which will magically remove the error for pytest, even after uninstall and reinstall using regular pip install.

So I think the best move is to use -e in the workflow for this package.

@sbillinge
Copy link
Contributor

Thanks for the hard work on this. This makes me pretty uncomfortable. Doesn't it mean if someone pip installs it, it won't run on linux?

Let's go with this workaround for now, but write an issue to revisit this and fix it properly. Could you also paste the failed tests trace into the issue so that we have a head-start when we do look at it?

Thanks,

S

@sbillinge sbillinge merged commit 50617fa into diffpy:main Aug 27, 2024
2 checks passed
@Tieqiong
Copy link
Contributor Author

Tieqiong commented Aug 27, 2024

In fact after further reading and testing, I think I know the reason now.

When simply running pytest, it will automatically find tests from the current dir (which would normally be something like dev/diffpy.pdffit2[call it pwd] as it's the working directory).

When we pip install , pip will compiled cpython extensions. Depend on installation mode, the cpython extensions will be in either pwd (-e install), or in the environment directory (something like minoconda3/envs/envs-name/.../site-package/diffpy/pdffit2[call it env])(regular install). These cpython files I believe are crucial files for the package.

So the problem is when we pip install . there's no cpython files in pwd, which pytest automatically find tests and run, leading to problems. By installing using -e, pip will build the cpython files in pwd so problems go away. When I uninstall -e and install it regularly the cpython files don't go away so pytest would still work, even the installed package itself is redirect to env.

Testing it in another way, when I pip install . and cd to env and copy-paste the pytest.ini file (which will help pytest find testing files by matching file name patterns), pytest will run flawlessly (given the fact that tests folder is included in the distribution for this package for some reason, probably MANIFEST.in).

In fact this doesn't limit to linux, I realize this is also happening on Mac. However I was not aware of cpython files at the beginning so there always exists a cpython in my pwd dir and I never deleted it to run a clean test.

Another thing I was wrong was cpython files are not in tests dir but pdffit2 dir. This is how modules can run import diffpy.pdffit2.pdffit2 even though there's no pdffit2 module in pdffit2. The cpython extension is the pdffit2 module. This also align with the circular import error, as modules can't find pdffit2 module if cpython is not built for that dir.

@sbillinge so in short, when the package involve with cpython extensions, we should run pytest at the directory where the package is actually built. So if -e install then pytest is fine as both are in pwd dir. Else if regular install then we need to pytest in env dir.

I think this is only a pytest thing, so fortunately it might not be that worrying...

@sbillinge
Copy link
Contributor

good sleuthing.

I think this means that for packages with c in them we may need a run_tests.py or something back in the top level directory? We used to have these and I didn't really know what they did so we cleaned them out and everything seemed to work still. Is there a solution where if there is a run_tests.py present things can be made to work? Then we could put it back in the cookiecutter with everything commented out, but we uncomment it for c-packages, or else we leave it uncommented and it just works either way?

@Tieqiong Tieqiong deleted the buildwf branch October 2, 2024 13:16
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