-
Notifications
You must be signed in to change notification settings - Fork 29
Change Notebooks to local installation of latest dev state #690
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: main
Are you sure you want to change the base?
Conversation
|
Also Fixes #689 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
=======================================
Coverage 45.96% 45.96%
=======================================
Files 55 55
Lines 8012 8012
=======================================
Hits 3683 3683
Misses 4329 4329
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "source": [ | ||
| "if IN_COLAB:\n", | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo" | ||
| " # you can install the latest release of assume-framework with !pip install assume-framework\n", |
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.
Should we not do this consistently, then, to avoid future errors? If we decide to install the dev state, do we always?
| "if IN_COLAB:\n", | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo" | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo\n", | ||
| " !pip install -e ./assume-repo[learning]" |
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.
It is weird that we do not install assume in the section that is called installation process. I would suggest restructuring the chapter as well
| "# Only run this cell if you are using Google Colab\n", | ||
| "if IN_COLAB:\n", | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo" | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo\n", |
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.
The installation command in the markdown cell still references the normal installation, there also with ". -e" then?
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.
This happens in multiple tutorials. Please make changes accordingly.
kim-mskw
left a comment
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.
Thank you for fixing this. I have a few remarks regarding consistency:
- I would suggest using the dev state in all tutorials, regardless of whether it is currently required. This helps avoid future issues and keeps everything consistent.
- Related to consistency: I know it’s a bit tedious, but please also update the respective markdowns in the tutorials.
Some of them still do not reference the dev state, and they mention that installation cells do not need to be executed if you already have the package installed.
However, with only the latest release installed, you do need to run the installation cells again to obtain the dev version.
| "# Only run this cell if you are using Google Colab\n", | ||
| "if IN_COLAB:\n", | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo" | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo\n", |
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.
This happens in multiple tutorials. Please make changes accordingly.
| "if IN_COLAB:\n", | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo" | ||
| " !git clone --depth=1 https://github.com/assume-framework/assume.git assume-repo\n", | ||
| " !pip install -e assume-repo" |
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.
Adjust the comment above that the code does not need to be executed when one has the current dev state installed, not the release.
| } | ||
| }, | ||
| "outputs": [], | ||
| "outputs": [ |
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.
clear outputs please and push again :)
|
@assume-framework/core-devs This is bigger than initially thought and made me think of our approach to versioning:
After discussing this with Ramiz, we came up with 3 potential solutions:
I would be in favor of 1) but thinking about consistent versioning seems to be a good idea to me anyway. What are your thoughts? |
|
@gugrimm I think you mentioned some additional constraints, which makes this problem look larger than I hope it is :D If users installed assume with btw when do you need admin rights to run the tests or something?
they are the same tag, same code, published through and from GitHub Actions? I would go with 3. - we take care of
as the local installed version should always match the notebook, this should always result in a working state (even if not the latest state - but well, running |
|
Now that we have setuptools-scm with dynamic versioning included (#701), it should be possible to print warnings if the installed assume version on users local machines does not match the repository straucture of the latest dev state. This would avoid users to trap into problems. |
Related Issue
Closes #678
Description
This changes the notebooks when executed in colab.
The motivation is to avoid errors that occur when both the latest release from pypi and the assume git repo are used in one notebook and the release is behind the git repo in terms of namings, data structure etc.
The proposed solution is to replace
!pip install assume-frameworkby!pip install -e .in all notebooks, where we clone the git repo in order to have access to tutorial data.Checklist