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

Merged content of draft-PR to dev #950

Closed
nailend opened this issue Sep 22, 2022 · 13 comments · Fixed by #870
Closed

Merged content of draft-PR to dev #950

nailend opened this issue Sep 22, 2022 · 13 comments · Fixed by #870
Assignees
Labels
🔥 urgent High priority. Blocks other stuff. 🙏 help wanted Extra attention is needed

Comments

@nailend
Copy link
Contributor

nailend commented Sep 22, 2022

@gnn @IlkaCu @nesnoj I just merged #826 into the dev and realized afterwards that my code includes some changes from unfinished #870 as we use a common table that recieved a egon_ prefix...
I therefore stop now and will not merge #915 and #924 as I don't know how to best handle this situation. I am sorry

@nailend nailend added 🙏 help wanted Extra attention is needed 🔥 urgent High priority. Blocks other stuff. labels Sep 22, 2022
@nesnoj
Copy link
Member

nesnoj commented Sep 23, 2022

Ok, I propose to reset it to the latest commit before your merge and merge your stuff later then - I'll have a look

@nesnoj
Copy link
Member

nesnoj commented Sep 23, 2022

I'm wondering why there's no merge commit, did you use fast-forward?

I'll merge #826, #915, #924 into CI now to be able to start the run. We can fix the dev later. As I'm not working (haha, no, these lines are written by my bot, really), I cannot take care of the dev problem today.
Maybe @gnn can have a look?

@nesnoj
Copy link
Member

nesnoj commented Sep 23, 2022

So please @IlkaCu @khelfen @gnn @AmeliaNadal @ClaraBuettner do not merge the broken dev into your feature branches until it will have been fixed!

@khelfen
Copy link
Contributor

khelfen commented Sep 23, 2022

oh damn, I may already have done that 😕

@nesnoj
Copy link
Member

nesnoj commented Sep 23, 2022

oh damn, I may already have done that confused

Yes, we just noticed... discussing solutions

@nesnoj
Copy link
Member

nesnoj commented Sep 23, 2022

@nailend and I had a look. We noted that it is even worse: some commits done in #870 are already in dev and we don't know from which merge they origin from. So the dev is supposed to be already "corrupted" right now (in a sense that runs will fail).

Possible solutions from our point of view:

  1. Revert changes back to a working version: not impossible but really hard to find in this merge hell. We were not able to clearly identify after investigation.
  2. Merge Features/#857 store intermediate results of heat demand timeseries #870 to dev, even if the PR is still a draft (or @ClaraBuettner will finish beforehand). As current dev includes (at least some) compromising commits anyways, this seems to be the preferable way. This would save us from reverting the problematic commits contained in other branches (dev has been merged into many other branches already).

Let's discuss further measures on Monday.

Nevertheless, the CI can be run today - #377 (comment)

@khelfen
Copy link
Contributor

khelfen commented Sep 23, 2022

FYI, yeah when running my branch with the faulty dev (which of course I also merged into CI) I get the following error in heat_etrago.buses:

ERROR - (psycopg2.errors.UndefinedTable) relation "demand.egon_etrago_timeseries_individual_heating" does not exist
LINE 7:                 demand.egon_etrago_timeseries_individual_hea...
                        ^

[SQL: 
            SELECT ST_Centroid(geom) AS geom
            FROM grid.
            egon_mv_grid_district
            WHERE bus_id IN (
                SELECT bus_id FROM 
                demand.egon_etrago_timeseries_individual_heating
                WHERE scenario = 'eGon2035')
            ]
(Background on this error at: http://sqlalche.me/e/13/f405)
Traceback (most recent call last):
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1276, in _execute_context
    self.dialect.do_execute(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/default.py", line 608, in do_execute
    cursor.execute(statement, parameters)
psycopg2.errors.UndefinedTable: relation "demand.egon_etrago_timeseries_individual_heating" does not exist
LINE 7:                 demand.egon_etrago_timeseries_individual_hea...
                        ^


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 984, in _run_raw_task
    result = task_copy.execute(context=context)
  File "/storage/eGon-data-de-kh/eGon-data/src/egon/data/datasets/__init__.py", line 194, in skip_task
    result = super(type(task), task).execute(*xs, **ks)
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/airflow/operators/python_operator.py", line 113, in execute
    return_value = self.execute_callable()
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/airflow/operators/python_operator.py", line 118, in execute_callable
    return self.python_callable(*self.op_args, **self.op_kwargs)
  File "/storage/eGon-data-de-kh/eGon-data/src/egon/data/datasets/heat_etrago/__init__.py", line 575, in buses
    insert_buses("rural_heat", scenario="eGon2035")
  File "/storage/eGon-data-de-kh/eGon-data/src/egon/data/datasets/heat_etrago/__init__.py", line 67, in insert_buses
    mv_grids = db.select_geodataframe(
  File "/storage/eGon-data-de-kh/eGon-data/src/egon/data/db.py", line 211, in select_geodataframe
    gdf = gpd.read_postgis(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/geopandas/io/sql.py", line 154, in _read_postgis
    df = pd.read_sql(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/pandas/io/sql.py", line 628, in read_sql
    return pandas_sql.read_query(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/pandas/io/sql.py", line 1579, in read_query
    result = self.execute(*args)
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/pandas/io/sql.py", line 1424, in execute
    return self.connectable.execution_options().execute(*args, **kwargs)
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 2235, in execute
    return connection.execute(statement, *multiparams, **params)
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1003, in execute
    return self._execute_text(object_, multiparams, params)
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1172, in _execute_text
    ret = self._execute_context(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1316, in _execute_context
    self._handle_dbapi_exception(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1510, in _handle_dbapi_exception
    util.raise_(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1276, in _execute_context
    self.dialect.do_execute(
  File "/home/local/RL-INSTITUT/kilian.helfenbein/anaconda3/envs/d_py38_egon_data_de/lib/python3.8/site-packages/sqlalchemy/engine/default.py", line 608, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.ProgrammingError: (psycopg2.errors.UndefinedTable) relation "demand.egon_etrago_timeseries_individual_heating" does not exist
LINE 7:                 demand.egon_etrago_timeseries_individual_hea...
                        ^

[SQL: 
            SELECT ST_Centroid(geom) AS geom
            FROM grid.
            egon_mv_grid_district
            WHERE bus_id IN (
                SELECT bus_id FROM 
                demand.egon_etrago_timeseries_individual_heating
                WHERE scenario = 'eGon2035')
            ]
(Background on this error at: http://sqlalche.me/e/13/f405)

@ClaraBuettner
Copy link
Contributor

I deleted everything related to individual heat in 517ab19 . The results look good in test mode, I will have a closer look today in the afternoon.
I will also merge this into CI hopefully today. But since I deleted some tasks, I'm not sure if the DAG needs to be started again on the server.

The issue with the duplicated tasks for individual heating only exists on CI, neither the dev nor my feature branch has the same problem. I assume that I did that when I merged the changes for parallelization, sorry!
I can easily fix this, but I would need to make a commit directly on CI. Is that possible?

@nesnoj
Copy link
Member

nesnoj commented Sep 28, 2022

I deleted everything related to individual heat in 517ab19 . The results look good in test mode, I will have a closer look today in the afternoon.

👍

I will also merge this into CI hopefully today. But since I deleted some tasks, I'm not sure if the DAG needs to be started again on the server.

If no one has objections, I'd say: give it a try! (there's not much to ruin ;)

The issue with the duplicated tasks for individual heating only exists on CI, neither the dev nor my feature branch has the same problem. I assume that I did that when I merged the changes for parallelization, sorry! I can easily fix this, but I would need to make a commit directly on CI. Is that possible?

Yes, go for it! (keep in mind the other manual commits @gnn did last Friday in the CI)

@ClaraBuettner
Copy link
Contributor

The CI-pipeline is running again.
I pushed all my changes to CI but manually did them on the server again to avoid including all other changes and have less problems with deleted tasks.
I will take care of finishing and merging #870 into dev tomorrow.

@nesnoj
Copy link
Member

nesnoj commented Sep 30, 2022

Yay, a lot happening in the pipeline 😄
Can the PRs mentioned in the initial post now be merged too? @ClaraBuettner
If so, could you please take care of this @nailend ?

@ClaraBuettner
Copy link
Contributor

Yay, a lot happening in the pipeline smile Can the PRs mentioned in the initial post now be merged too? @ClaraBuettner If so, could you please take care of this @nailend ?

I merged #870, so the dev should be fixed and other branches can be merged again (I also started merging other PRs).

@gnn
Copy link
Collaborator

gnn commented Sep 30, 2022

The issue with the duplicated tasks for individual heating only exists on CI, neither the dev nor my feature branch has the same problem. I assume that I did that when I merged the changes for parallelization, sorry! I can easily fix this, but I would need to make a commit directly on CI. Is that possible?

TL;DR: What @nesnoj said.

But I'd also like to elaborate. Committing directly to the CI is possible, given that the CI is just a regular branch which can be pulled from, edited and pushed to. It is however something which should normally be avoided since the whole point of the CI is to test the intermediate work on other branches without any additional commits confounding the test results. The current state of the CI is rather special though, since it seems that it is in a state of brokenness which we DO NOT want to reproduce on dev. So having a special commit which fixes this brokenness and which is unique to the CI is a valid exception to the guidelines above.

Yes, go for it! (keep in mind the other manual commits @gnn did last Friday in the CI)

The CI-pipeline is running again. I pushed all my changes to CI but manually did them on the server again to avoid including all other changes and have less problems with deleted tasks. I will take care of finishing and merging #870 into dev tomorrow.

Concerning my "manual" changes and what you had to do with manually replaying your changes, @ClaraBuettner, there actually are no manual changes done by me on the server and there's infrastructure for "cherry-picking" certain commits to be replayed on the server. In short, you can use git format-patch to write out certain commits into files. Then you can drop those files into the "git-repos/friday-evening-weekend-run/patches/" directory and they will be replayed on top of the CI before it gets started. If you start it with the shell script that creates a fresh checkout and working directory before it starts the workflow, that is.
I'll elaborate later, just don't have the time right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 urgent High priority. Blocks other stuff. 🙏 help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants