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 colorlog import #1482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maurerle
Copy link
Contributor

Reason for the proposed changes

I get the error that colorlog is not valid.
This seems to have change in one of the latest versions of the package.

This issue occurs when installing with pip install -e .

INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=master

@fievelk
Copy link
Contributor

fievelk commented Jul 21, 2022

Thanks for the PR!
I believe that the issue here is mostly that gsy-e-sdk and gsy-e are enforcing two different versions of colorlog.

So, before we make this change, I think we should either bump colorlog to the latest version (6.6.0) or set gsy-e-sdk to use 4.7.2 too. I personally suggest to bump the version to the latest release and check if this creates incompatibilities between repos. I'll report the issue on slack.

@maurerle
Copy link
Contributor Author

Well, wouldn't it be useful to losen those hard constraints anyway to allow flexible integration with other packages?
We had a lot of worries with the strict dependencies.

I'd remove the fixed dependencies so that an pip install -e . works either for users - where its needed you can still create the fixed state with fixed versions then, but other users have less problems using this project.

@fievelk
Copy link
Contributor

fievelk commented Jul 21, 2022

Well, wouldn't it be useful to losen those hard constraints anyway to allow flexible integration with other packages? We had a lot of worries with the strict dependencies.

I'd remove the fixed dependencies so that an pip install -e . works either for users - where its needed you can still create the fixed state with fixed versions then, but other users have less problems using this project.

@maurerle I'd personally keep some form of constraint. If we loosen them completely, once the external library implements breaking changes it will cause exceptions on our end (also on production). What I would suggest is in-between: we can bump using a "compatible release" specifier (see PEP 440). For example, colorlog~=6.6.0 would allow us to automatically bump the micro version. We could even try with ~=6.6 and bump the minor version.

@maurerle
Copy link
Contributor Author

Sounds good, but this is AFAIK not possible using pip-compile from pip-tools.

Maybe you can look at our fork to see how we managed to be flexible when needed by installing from base.in:
https://github.com/bc4p/gsy-e/blob/master/setup.py#L10
but also provide a fixed version with our additional dependencies as a feature.

I would love to integrate this into the upstream GSY.

Furthermore, if you update gsy-framework, you have to directly update the repositories depending on it accordingly - as you are installing gsy-framework directly from master as a dependency. This is a major source for breaking.
Especially, as gsy-framework also needs fixed dependencies.
This becomes unmanagable if you try to install gsy-e from an old commit, as the latest gsy-framework is still to be used.

I would suggest to use git submodules there - as this allows to specify an explicit commit-id.
This is quite generic and does not include the repository name if done right:
https://github.com/bc4p/gsy-e/blob/master/.gitmodules

What do you think about this?

@fievelk
Copy link
Contributor

fievelk commented Aug 1, 2022

Sounds good, but this is AFAIK not possible using pip-compile from pip-tools.

@maurerle in my experience, this is possible. We should just update base.in here

colorlog==4.7.2

to be

colorlog~=6.6.0 

Then running fab compile (or using pip-tools directly) should work. Of course, these changes should have been made on gsy-framework too, otherwise there might be other compatibility issues. Have you already given it a try?

Furthermore, if you update gsy-framework, you have to directly update the repositories depending on it accordingly - as you are installing gsy-framework directly from master as a dependency. This is a major source for breaking.

I personally agree with this, but there might be other more straightforward ways to deal with it than using submodules. One way could be using tags or commits' hashes to point to the relevant gsy-framework commit. But this requires some time and effort on the team. I've raised the issue and created a ticket for it.

@maurerle maurerle force-pushed the maurerle-fix-colorlog branch from e6d2b5c to 8137962 Compare March 15, 2024 22:02
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.

2 participants