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

MAINT: Test against installed package #12280

Closed
wants to merge 2 commits into from

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Dec 8, 2023

@hoechenberger
Copy link
Member

Nice, thanks @larsoner

x-ref #12281

@hoechenberger
Copy link
Member

hoechenberger commented Dec 8, 2023

@larsoner
I think it's worthwhile considering switching to a src/ layout to further reduce the chances of erroneously using mne in the CWD instead of the installed version. The current layout also caused the entire "VS Code cannot handle editable installs anymore" thing to go unnoticed.

Thoughts?

@larsoner
Copy link
Member Author

larsoner commented Dec 8, 2023

It sounds like a pretty big change and a departure from how most scientific python projects (numpy/scipy/pandas etc., but there are exceptions like matplotlib) are organized AFAIK. I'd rather move slower than faster here.

@drammock
Copy link
Member

drammock commented Dec 8, 2023

I think it's worthwhile considering switching to a src/ layout to further reduce the chances of erroneously using mne in the CWD instead of the installed version.
It sounds like a pretty big change and a departure from how most scientific python projects (numpy/scipy/pandas etc., but there are exceptions like matplotlib) are organized AFAIK. I'd rather move slower than faster here.

I'm actually less swayed by the "what does numpy do" heuristic here, perhaps because there are exceptions (pydata-sphinx-theme is another one). For me the relevant question is "is it likely to continue causing problems for us?" We've hit two big-ish and annoying problems, and now have fixes for both of them. The biggest cost may be that changing from mne/ to src/ will require our devs to update their muscle memory for actions that some of them make tens or hundreds of times per day. So I don't want to incur that cost unless we can foresee future problems. Might be worth asking for opinions on the scientific python discord or discourse?

@@ -12,6 +12,13 @@ if [ "${MNE_CI_KIND}" == "notebook" ]; then
else
USE_DIRS="mne/"
fi
JUNIT_PATH="junit-results.xml"
if [ ! -z "$CONDA_ENV" ]; then # use installed verison
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if [ ! -z "$CONDA_ENV" ]; then # use installed verison
if [ ! -z "$CONDA_ENV" ]; then # use installed version

@hoechenberger
Copy link
Member

@larsoner Are you sure this is actually testing what we want here?

I'd much rather ship our tests with the sdist, rm -rf the checked-out repository, and run the tests from site-packages/mne just to be absolutely sure… No?

@larsoner
Copy link
Member Author

larsoner commented Dec 8, 2023

@larsoner Are you sure this is actually testing what we want here?

We aren't, and I'm trying to figure out how to make it work but it's not trivial it seems

I'd much rather ship our tests with the sdist

Agreed we should do this, I think we used to so it's weird to me to exclude them now

rm -rf the checked-out repository, and run the tests from site-packages/mne just to be absolutely sure… No?

Yes this could work... except that we don't ship the test files with the sdist (or wheel) for space reasons. So we'd need to copy over the test data files to the installed location first.

@hoechenberger
Copy link
Member

Let's include the test files with the sdist, then

I'd much rather ship our tests with the sdist

Agreed we should do this, I think we used to so it's weird to me to exclude them now

I excluded the tests because most wouldn't work anyway without the testing files.

rm -rf the checked-out repository, and run the tests from site-packages/mne just to be absolutely sure… No?

Yes this could work... except that we don't ship the test files with the sdist (or wheel) for space reasons. So we'd need to copy over the test data files to the installed location first.

Let's move the testing files to an external dataset

@hoechenberger
Copy link
Member

FWIW including the testing files in the sdist makes it grow from 14 to 55 MB on my system. Nothing I would consider outrageous these days anymore.

@larsoner
Copy link
Member Author

larsoner commented Dec 8, 2023

See also #4926, feel free to reopen if you see a good path forward

@larsoner larsoner closed this Dec 8, 2023
@larsoner larsoner deleted the test-package branch December 8, 2023 18:30
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.

3 participants