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

Create a python package of flexmeasures-entsoe #33

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

Conversation

Ahmad-Wahid
Copy link
Contributor

In this PR, I have tried to create a pip installable package of our entsoe plugin. I have also removed the unused python modules, and solved the mypy issue.

@Ahmad-Wahid Ahmad-Wahid requested a review from nhoening August 2, 2023 17:10
@Ahmad-Wahid Ahmad-Wahid linked an issue Aug 2, 2023 that may be closed by this pull request
@Ahmad-Wahid Ahmad-Wahid self-assigned this Aug 2, 2023
@Ahmad-Wahid Ahmad-Wahid added the enhancement New feature or request label Aug 2, 2023
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.

Great, I tested installing locally and importing just via the name flexmeasures_entsoe.

I added two small improvements in the Readme myself.

For the detailled&pinned requirements (.txt), I wonder if they are needed. This is always going to be installed in addition to FlexMeasures, so I'd always use the .in versions. I'd otherwise easily run into clashed between the installed dependencies for FlexMeasures and the ones for the plugin. What do you think?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Ahmad-Wahid
Copy link
Contributor Author

Should I merge this PR?

@nhoening
Copy link
Contributor

We still need to decide on getting rid of the pinned dependencies (.txt). See my comment above.

@rhpijnacker
Copy link

Are there any plans to merge this in at some point?

@nhoening
Copy link
Contributor

nhoening commented Oct 6, 2024

Yes that would be great.
We have an internal deadline right now, but maybe afterwards we can return to this.

Was there anything blocking this, @Ahmad-Wahid ?

@Ahmad-Wahid
Copy link
Contributor Author

To get rid of the pinned requirements, should I just remove the files or the requirements?

@nhoening
Copy link
Contributor

nhoening commented Oct 7, 2024

The open question was if pinned dependencies should stay. Here are my thoughts:

  • I'd say yes, but they should not impact what the pip-install procedure would install. Apparently, this is already the case in this PR!
  • We can document in the Readme that make install-for-dev installs the exact .txt dependencies (so that devs work against the same stack), and that make upgrade-deps can upgrade these dependencies.
  • We should also document that importing just via the name flexmeasures_entsoe works.
  • Finally, a question: should we remove the dependency of flexmeasures from this plugin? It cannot be used on its own, it is installed by a flexmeasures installation. I think that is a good idea.

@Flix6x
Copy link
Contributor

Flix6x commented Oct 7, 2024

should we remove the dependency of flexmeasures from this plugin? It cannot be used on its own, it is installed by a flexmeasures installation.

I was also wondering about this with our smart-buildings plugin recently, where it isn't (yet) listed as a dependency, but I thought about including it. I haven't got a final answer, but just wanted to make an argument for including (keeping) the dependency: as a dev, if flexmeasures is installed in my local environment, it helps my IDE find imported functions and read their docstrings. So maybe include it as a dev dependency only?

@nhoening
Copy link
Contributor

nhoening commented Oct 7, 2024

Yes, we can try that.
Otherwise, for the actual dependency checking, I think it is clean to just list what the plugin requires on top of FlexMeasures' stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make a real package
4 participants