feat: modify copy commit processing rule #237
Conversation
|
@energyLS please review! :) |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Hey @davide-f :) |
|
@davide-f tried to implement the review aspects like in pypsa-earth: Somehow it doesnt work here. As soon as we import scripts.helpers in the Snakefile of pypsa-earth sec we get an error that we dont find the module scripts._helpers in the Snakefile of pypsa earth. I dont understand why this is happening. Do you have an idea? :) |
|
As additional comment, in PyPSA-Earth we do have: May be good to check if that's necessary to have in pypsa-earth and whether it's absence in -sec may lead to issues Unfortunately these weird behaviours are most likely due to the subworkflows that are not well maintained... |
|
Hi @davide-f,
adding it in the snakefile of pypsa-earth-sec, which didnt work. |
scripts/helpers.py
Outdated
| _logger = logging.getLogger(__name__) | ||
| try: | ||
| last_commit_message = ( | ||
| subprocess.check_output( | ||
| ["git", "log", "-n", "1", "--pretty=format:%H %s"], | ||
| cwd=path, | ||
| stderr=subprocess.STDOUT, | ||
| ) | ||
| .decode() | ||
| .strip() | ||
| ) | ||
| return last_commit_message | ||
| except subprocess.CalledProcessError as e: | ||
| _logger.warning(f"Error executing Git: {e}") | ||
| return None |
There was a problem hiding this comment.
According to the documentation, it seems that the cwd option overrides the cwd.
It may be a good idea to create a backup of cwd and restore it after the execution in an error-proven way.
To do this, for example, we may:
- before the try, initialize the last_commit_message to None and store cwd in a local variable
- in the try/except, drop the return statements
- after the try/except, restore the original cwd path
- return the last_commit_message
This may be worth trying to check if it solves the problem.
It may be good to reflect these changes into pypsa-earth as well, once validated.
There was a problem hiding this comment.
Hi @davide-f ,
thanks for the solution approach. Sadly it seems not to work.
Do you have any other ideas? Im pretty much empty with solution ideas.
for more information, see https://pre-commit.ci
|
Hello! :D So, I've locally tested your PR and I made it run by removing the "scripts." in the pypsa-earth Snakefile in the imports like "from scripts._helpers". I think we are close to finalize :) |
|
Hi @davide-f :) |
|
Hi @davide-f :D |
|
super :) CI is also passing. |
@davide-f then I will merge it? :) |
Yeah :D |
Closes # (if applicable).
Changes proposed in this Pull Request
Added a copy commit function, which stores the commit id and commit message of pypsa earth sec and the submodule in the n.meta of the .nc file
Checklist
envs/environment.yamlandenvs/environment.docs.yaml.config.default.yaml,config.tutorial.yaml, andtest/config.test1.yaml.doc/configtables/*.csvand line references are adjusted indoc/configuration.rstanddoc/tutorial.rst.doc/release_notes.rstis amended in the format of previous release notes, including reference to the requested PR.