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

Fix deleting beliefs in CLI #1095

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Ragnar-the-mighty
Copy link

@Ragnar-the-mighty Ragnar-the-mighty commented Jun 12, 2024

fixes #1092 1092

There are more places that has same issues but I do not have test cases so I only fixed this one.

Description

The cli command flexmeasures delete prognoses
returns
TypeError: %d format: a real number is required, not CursorResult

The reason is that after move to SQLAlchemy 2 the code
num_forecasts_deleted = db.session.execute(query)
Does not work as it use to do.

The fix is to use .rowcount as recommended by SQLAlchemy 2 documentation

Look & Feel

No change in UI
No change in CLI parameter or API endpoints

How to test

Call the cli command
flexmeasures delete prognoses
It should no longer throw the
TypeError: %d format: a real number is required, not CursorResult

Further Improvements

There are more instances of
num_forecasts_deleted = db.session.execute(query)
But I am a newbee and I do not have test cases for the other instances.

Related Items

This should close issues #1092


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

After move to SQLAlchemy 2  the code
num_forecasts_deleted = db.session.execute(query)
Does not work as it use to.
Changed code to use .rowcount

There are more places that has same issues but I do not have test cases so I only fixed this one.
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Thanks for helping to fix this!

Let's fix the other instances as well, and also the flexmeasures delete beliefs command I mentioned in #1092

We can do this, especially because with the help of this method of adding data we can create whatever we want, then attempt to delete it :)

@Ragnar-the-mighty
Copy link
Author

OK, I try to fix the other instances and the also the flexmeasures delete beliefs command

@nhoening
Copy link
Contributor

Thanks!

@nhoening nhoening changed the title Fix issues #1092 Fix deleting beliefs in CLI Jun 17, 2024
@Ragnar-the-mighty
Copy link
Author

Ragnar-the-mighty commented Jun 18, 2024

Hmm, I did not change flexmeasures/cli/data_delete.py and the flexmeasures delete beliefs command work for me.

I am not trusting my code changes and I do not want to cause work for anyone else.
I appreciate the help I got to unblock myself.
I will suspend work on PR until I have more confidence that I understand what I am doing.

@nhoening
Copy link
Contributor

Yes, flexmeasures delete beliefs also works for me, not sure what the difference is. I also got flexmeasures delete unchanged-beliefs to work.

Then this PR should simply concentrate on flexmeasures delete measurements (which I tested to have the same bug you reported). Can you apply the fix there, we well? Then I believe we did a good step with this PR.

@Ragnar-the-mighty
Copy link
Author

Nicolas, thank you for testing and merging the code, I hope that I will be able to do test better and fix any issues blocking merge

@nhoening
Copy link
Contributor

I believe the remaining work would be to simply repeat what you did for one command in the other one.

@nhoening
Copy link
Contributor

Hi @Ragnar-the-mighty are you able to fix the other command we talked about as well (flexmeasures delete measurements)? I guess that is all we should be doing here and then we can merge.

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.

flexmeasures delete prognoses fails with TypeError
2 participants