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

Remove jinja2 and pooch from sys_info #12411

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Feb 2, 2024

Fixes #12387.

@cbrnr cbrnr changed the title Remove jinja2 and pooch from sys_info Remove jinja2 and pooch from sys_info Feb 2, 2024
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Works for me. I think in principle maybe we could want all core dep versions listed somewhere, but in practice over the many years we've had this I don't think these have ever been useful to have reported. So I'm okay with removing them in favor of trimming things down a bit. But @drammock you had suggested possibly moving them rather than removing them so I'll let you look before merge

@cbrnr
Copy link
Contributor Author

cbrnr commented Feb 2, 2024

I'm also in favor of trimming down the output to things that will be relevant for us. It is unlikely that a bug is caused by pooch or jinja2 at the end of the day. Of course, we already have a dependencies="developer" option, where we could also move these packages to.

@larsoner
Copy link
Member

larsoner commented Feb 2, 2024

Of course, we already have a dependencies="developer" option, where we could also move these packages to.

Works for me and might satisfy @drammock's desire to keep them somewhere (he had suggested misc I think but maybe lumping under developer is good enough)

@drammock
Copy link
Member

drammock commented Feb 2, 2024

including under developer sounds ideal to me. IIRC there was a brief period when pooch version mattered, which could in theory happen again (e.g., if they decided to do a major API change)

Copy link
Contributor Author

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

I added the other required packages to a new section called "Infrastructure". Well, all but lazy_loader, which doesn't have a version attribute.

@drammock
Copy link
Member

drammock commented Feb 2, 2024

pushed a commit to hopefully fix Circle, which seems to have been broken by docdict changes in #12195

cc @withmywoessner; see 43094a7; I think the only necessary changes were the ones relating to blank lines, but I took the opportunity to clean up the formatting a bit too

@drammock drammock enabled auto-merge (squash) February 2, 2024 17:45
@drammock
Copy link
Member

drammock commented Feb 2, 2024

reported the missing lazy_loader version upstream: scientific-python/lazy-loader#96

@drammock
Copy link
Member

drammock commented Feb 2, 2024

pip-pre failure is being tracked in #12413

@larsoner larsoner merged commit 57611ae into mne-tools:main Feb 2, 2024
27 of 30 checks passed
@larsoner
Copy link
Member

larsoner commented Feb 2, 2024

Thanks @cbrnr !

@cbrnr cbrnr deleted the sysinfo branch March 12, 2024 10:08
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing jinja2 from mne.sys_info()
4 participants