Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Remove websockets-client dependency #2878

Closed

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Apr 2, 2021

Description

The websocket client dependency is now handled by mycroft-messagebus-client and not needed here anymore.

Contributor license agreement signed?

CLA [ Yes ]

The websocket client dependency is now handled by mycroft-messagebus-client and not needed here anymore.
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Apr 2, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

Copy link
Contributor

@clusterfudge clusterfudge left a comment

Choose a reason for hiding this comment

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

I believe that it's a common practice (citation needed) to include transitive dependencies in a requirements.txt file, for the purpose of pinning fixed conflicts (as well as simple population via pip freeze). A model that's starting to gain some traction is "requirements.in + requirements.lock", where .in captures your direct dependencies, and .lock captures locked versions of the entire dependency graph. There's a decent write-up here.

@ChanceNCounter
Copy link
Contributor

I'm not sure that's a concern for software that's meant to be in a virtual environment. 2nd+ degree dependencies will go away with the venv, if it's nuked and recreated, and unfrozen deps upstream would need the stars to align to hit users.

It wouldn't hurt, but it's extra dev steps to avoid a situation that would arise if, and only if, a user re/installed from scratch, while a dep is broken due to breaking changes in an unfrozen dep of its own, before either MycroftAI or Mycroft's dep notice and freeze the problem package.

@kjcole
Copy link

kjcole commented Apr 2, 2021

I believe that it's a common practice (citation needed) to include transitive dependencies in a requirements.txt file, for the purpose of pinning fixed conflicts (as well as simple population via pip freeze). A model that's starting to gain some traction is "requirements.in + requirements.lock", where .in captures your direct dependencies, and .lock captures locked versions of the entire dependency graph. There's a decent write-up here.

FWIW, I've grown quite fond of pipenv and, following the write-up you pointed to links to Overview of python dependency management tools which also seems to endorse pipenv, as well as poetry...

@forslund
Copy link
Collaborator Author

forslund commented Apr 2, 2021

I do think the versioned package control is important to keep control over exact versions and the pip-compile looks quite useful especially for creating repeatable builds.

In this specific case the version of mycroft-messagebus-client keeps a locked version of the websocket-client and it shouldn't need to be here as well.

Renaming the requirements.txt to requirements.in in the future and adding the .lock file sounds like a good idea. dev setups just need to worry about the .in file while the packagings for mark-1 and mark-2 can make use of the .lock file to make the builds 100% repeatable. Previously pipenv has also been discussed as a possible improvement and seems like it would accomplish the same task.

@clusterfudge
Copy link
Contributor

Ok, it sounds like y'all are pretty thoughtful on the topic, so please consider my commentary non-blocking! Happy to see us make improvements in the future.

@forslund
Copy link
Collaborator Author

forslund commented Apr 7, 2021

Closing this in favor of #2879

@forslund forslund closed this Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants