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

Complete restructuring of the SupplyChain module #66

Merged
merged 64 commits into from
Nov 8, 2023

Conversation

aleeciu
Copy link
Collaborator

@aleeciu aleeciu commented Jan 27, 2023

This pull request features a complete restructuring of the supply chain module, including:

@aleeciu
Copy link
Collaborator Author

aleeciu commented Jan 27, 2023

Hi @emanuel-schmid, in this pull request I am using the pymrio module (https://pymrio.readthedocs.io/en/latest/index.html) which is currently not included in the CLIMADA environment. How shall I add it?

@emanuel-schmid
Copy link
Contributor

Yes, this problem is currently not really solved.

There are workarounds though: one can add a line pip install pymrio in script/branches/Jenkinsfile. With this, the environment is updated before running unit-tests on Jenkins. (I just did this and the tests ran through)

However, this file should actually remain unchanged in the develop branch and requirements/env_climada.yaml should be changed instead. You can update env_climada.yaml anytime in this branch but it won't have an effect on the environment, which is defined by the develop branch alone. On the other hand, the changes in the Jenkinsfile be undone before or after merging. (They don't hurt but the slow down the unittests for a couple of seconds for each commit.)

For now I suggest to proceed like this and I'll clean up the Jenkinsfile after the PR has been merged.

@aleeciu
Copy link
Collaborator Author

aleeciu commented Feb 1, 2023

Great, thanks @emanuel-schmid for the clarification. I will then update the requirements/env_climada.yaml in develop after merging. One more question: I keep getting a readthedocs error but I am not quite sure how to fix/interpret it.

@aleeciu aleeciu requested review from peanutfun and zeliest February 1, 2023 17:51
@peanutfun
Copy link
Member

I keep getting a readthedocs error but I am not quite sure how to fix/interpret it.

@aleeciu Try merging the latest version of develop back into this branch

@aleeciu
Copy link
Collaborator Author

aleeciu commented Feb 9, 2023

I keep getting a readthedocs error but I am not quite sure how to fix/interpret it.

@aleeciu Try merging the latest version of develop back into this branch

great, it worked thanks

@@ -23,6 +23,7 @@ pipeline {
sh '''#!/bin/bash
export PATH=$PATH:$CONDAPATH
source activate petals_env
pip install pymrio
Copy link
Member

Choose a reason for hiding this comment

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

@emanuel-schmid could you add pymrio to the Petals environment, so that we can remove this line here?

@aleeciu
Copy link
Collaborator Author

aleeciu commented Oct 13, 2023

Many thanks @peanutfun; I am confused because I saw some suggested changes from your side but now they are marked as resolved. It's all good for me if you implemented them directly, just please let me know if changes are still expected from my side.

ps
I did not mean to urge you few days ago by reminding you about the review - I just pressed the button by mistake :-)

@peanutfun
Copy link
Member

@aleeciu I now marked the comments as resolved. Some were outdated and for others I am not familiar enough with the module to mess with it now. If you are fine with how it works, I think we can merge 🙂

@aleeciu
Copy link
Collaborator Author

aleeciu commented Oct 13, 2023

we can merge from my side, and we still have #81 which will bring changes to this module

@emanuel-schmid emanuel-schmid merged commit 3184e83 into develop Nov 8, 2023
3 checks passed
@emanuel-schmid emanuel-schmid deleted the supplychain branch November 8, 2023 10:47
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