-
Notifications
You must be signed in to change notification settings - Fork 14
improved bash scripting for conda setup #72
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
Conversation
shankari
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.
@ryan-tock this largely copying over the setup from e-mission-server to mobilitynet. But that ends up changing the conda version, and more importantly the set of packages installed. Note that the original environment.yml had shapely and copied over environments don't.
That's because we use shapely for spatial trajectory evaluation in mobilitynet, but not (yet?) in production. As I said, we should really stick with the old conda version, at least initially, for better reproducibility. If that doesn't work, we can upgrade the conda version as well, but I would then expect the rationale for doing so to be documented (e.g. what was the error with the old version)
| target/ | ||
|
|
||
| # Conda | ||
| miniconda.sh |
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.
glad to see that you excluded this!
|
I have now downgraded to the previously used conda version with the previous environment |
shankari
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.
You can also rebase your changes (git rebase -i HEAD~3 or similar) instead of creating new commits to keep the commit history a bit cleaner.
setup/teardown.sh
Outdated
| @@ -1,2 +1 @@ | |||
| conda activate base | |||
| conda env remove --name emissioneval | |||
| conda deactivate && conda env remove --name emission | |||
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.
| conda deactivate && conda env remove --name emission | |
| conda deactivate && conda env remove --name emissioneval |
Those of us who also write server code already have an emission environment set up and we don't want to mess it up with a different set of dependencies
setup/setup.sh
Outdated
| conda env update --name emissioneval --file setup/environment.yml | ||
| conda activate emissioneval | ||
| echo "Installing using conda now" | ||
| conda env update --name emission --file setup/environment.yml |
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.
| conda env update --name emission --file setup/environment.yml | |
| conda env update --name emissioneval --file setup/environment.yml |
Ditto
|
I found all instances of emission and replaced them with emissioneval as well as doing a rebase. It should be ready now |
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.
Please submit another PR to update the README! You can improve the documentation and and fix the broken links at the same time
| conda create -n emissioneval | ||
| echo "Installing using conda now" | ||
| conda env update --name emissioneval --file setup/environment.yml | ||
| conda activate emissioneval |
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 noticed that you no longer activate right after the setup here.
But then I noticed that you do set this up as part of the new activate_conda.sh script
https://github.com/MobilityNet/mobilitynet-analysis-scripts/pull/72/files#diff-b6b89814c0d70733151b062c77846d8546d726d07548236ee6a5bacff96b1f5eR7
I am merging this now to unblock @KG2468, but note that you need to update the README to match
https://github.com/MobilityNet/mobilitynet-analysis-scripts/tree/master?tab=readme-ov-file#running-existing-notebooks
|
when I try to run the new setup script, I get an error |
|
This is because I had a prior partial install of miniconda that did not have mamba installed BUT was configured to use mamba by the previous run of the setup script. After re-running, it was able to install properly |
No description provided.