-
Notifications
You must be signed in to change notification settings - Fork 52
Startup quickfixes #1430
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
base: master
Are you sure you want to change the base?
Startup quickfixes #1430
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses startup issues for new users by adding the missing kachery-cloud dependency and improving the database configuration tutorial workflow.
- Added
kachery-cloudas an explicit dependency across all environment files - Refactored the database setup tutorial to pull configuration variables to the top for better visibility
- Fixed return type consistency in
AnalysisNwbfile.get_abs_pathmethod
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added kachery-cloud to project dependencies |
| environment.yml, environment_dlc.yml, environment_moseq_cpu.yml, environment_moseq_gpu.yml | Added kachery-cloud to pip dependencies in all environment files |
| notebooks/00_Setup.ipynb, notebooks/py_scripts/00_Setup.py | Restructured database setup tutorial to clarify configuration steps and ensure global config is saved |
| src/spyglass/common/common_nwbfile.py | Changed return type from Path to str for consistency |
| CHANGELOG.md | Added changelog entries for the dependency addition and return type fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "7ebcb0bf", | ||
| "metadata": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #17. "database.host": "lmf-db.cin.ucsf.edu",
Do we want this to be specific to our database?
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. this should be the database_host value defined above
Co-authored-by: Copilot <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1430 +/- ##
=======================================
Coverage 71.24% 71.24%
=======================================
Files 114 114
Lines 13248 13248
=======================================
Hits 9438 9438
Misses 3810 3810 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the migration to globally saved config, we could delete the chdir lines across all notebooks, to avoid carrying the vestige
I might also add a 'common error' section to 01_Concepts.ipynb along the lines of "Prompted for your username when you thought you saved your config? Find your settings with:"
print(dj.config) # Usually ./dj_local_conf.json or ~/.datajoint_config.json"To manually connect without a config..."
dj.conn(user="youruser", password="yourpass", host="yourhost")"If you think you may be having connection issues, try the above or contact admin to reset your password"
Could be this pr, could be an issue to resolve in the next
| database_user="your username", | ||
| database_password="your password", # remove this line for shared machines | ||
| database_host="localhost or lmf-db.cin.ucsf.edu", # only list one | ||
| save_method="global", # global or local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this line, let it use the default, global
| # "ghostipy", # removed from list bc M1 users need to install pyfftw first | ||
| "hdmf>=3.4.6", | ||
| "ipympl", | ||
| "kachery-cloud", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be in yaml and toml files? As an imported package, my thought would be to only list here in toml and let the yaml's - . handle installing for the conda env
| dj.config.save_global() | ||
| dj.config.save_local() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because save_dj_config should do this already, yeah? I might change this too a check-exists.
| dj.config.save_global() | |
| dj.config.save_local() | |
| your_config = Path.home() / '.datajoint_config.json' | |
| print(f"Config exists: {your_config.exists()}" | |
| from NWB #1433 | ||
| - Split `SpyglassMixin` into task-specific mixins #1435 #1451 | ||
| - Auto-load within-Spyglass tables for graph operations #1368 | ||
| - Add explicit `kachery-cloud` dependency #1430 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Add explicit `kachery-cloud` dependency #1430 | |
| - Add explicit `kachery-cloud` dependency #1430 | |
| - Default ton globally saved config #1430 |
Description
Fixes #1264
kachery-cloudas a dependency since no longer comes withsortingviewdj config setup tutorial
Checklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.