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 hydra plugin discovery #77

Merged
merged 4 commits into from
Aug 26, 2022
Merged

Fix hydra plugin discovery #77

merged 4 commits into from
Aug 26, 2022

Conversation

lemairecarl
Copy link
Contributor

@lemairecarl lemairecarl commented Aug 24, 2022

J'ai remarqué que vital n'utilisait pas cette ligne qui semble importante dans l'exemple de SearchPathPlugin :
https://github.com/facebookresearch/hydra/blob/main/examples/plugins/example_searchpath_plugin/setup.py#L17

Tu peux essayer ça de ton bord et me dire ce que ça donne.
Aussi, mystérieusement, on dirait que ça corrige les autres erreurs d'import (i.e. from vital import get_vital_home), mais ça reste à confirmer.

@lemairecarl
Copy link
Contributor Author

Une chose que je trouve bizarre, c'est que si vital rend un package hydra_plugins disponible à l'import, ça veut dire qu'il ne pourrait pas y avoir d'autres packages qui ont leur hydra plugins, car il y aurait un conflit... ?

setup.py Outdated Show resolved Hide resolved
Copy link
Member

@nathanpainchaud nathanpainchaud left a comment

Choose a reason for hiding this comment

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

Ça m'a l'air bien de façon générale et cela me semble être des ajouts nécessaires si jamais on veut être capable d'installer vital dans un mode autre qu'éditable, puisque dans ce cas les configs ne seraient effectivement pas inclues avec le reste du code dans le package.

Par contre, dans mon cas ces ajouts ne sont pas suffisants pour régler le problème avec la searchpath pour les configs de vital. Je serais donc de l'opinion de garder la PR ouverte tant que je n'arrive pas à trouver une solution qui réglera le bug de mon côté (et qui pourrait impliquer des changements ici).

@nathanpainchaud
Copy link
Member

Au final, j'ai même essayé de cloner à nouveau le repo d'un de mes projets qui utilise vital, de faire le checkout de ta branche puis d'installer l'environnement (pour répliquer une nouvelle installation), et le problème persiste de mon côté.

Au point où c'est rendu, je commence à vouloir jeter l'éponge de mon côté. C'est bien que de ton côté la solution fonctionne, parce que c'est toi qui a besoin de créer de nouveaux environnement. Je vais soigneusement sauvegarder les miens de mon côté, en espérant que la cause du problème soit un bug un peu obscur dans une des librairies qui participent au setup d'environnement/installation de package, et qu'éventuellement ça va être réglé upstream. Mais près 2 jours d'investigation sans avoir pu même comprendre l'origine exacte du problème, je pense qu'il faut que je passe à autre chose.

On pourra toujours se référer à cette PR si le problème surgit à nouveau dans le futur.

Copy link
Member

@nathanpainchaud nathanpainchaud left a comment

Choose a reason for hiding this comment

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

Je pense que pour les problèmes que j'ai spécifiquement concernant l'installation de vital, j'ai trouvé la cause dans ces deux issues de setuptools:

pypa/setuptools#3548
pypa/setuptools#3557

Donc si toi de ton côté le fix corrige le problème, je suis prêt à l'approuver. Je pense que l'origine de notre problème c'est les breaking changes d'installation en mode éditable de setuptools, mais que tu as trouvé un problème qu'on aurait eu plus tard si on avait voulu publier un package statique de vital. Donc tu es allé au devant d'éventuels bugs 😉

De mon côté, je vais surveiller l'évolution des issues ave setuptools et j'ouvrirai (probablement) des PRs dans le futur pour adapter le packaging de vital en fonction de comment les développeurs de setuptools vont réagir aux breaking changes. Entre temps, je pourrais toujours voir comment pinner setuptools<64 pour patcher temporairement le bug.

@lemairecarl lemairecarl merged commit 101c212 into dev Aug 26, 2022
@lemairecarl lemairecarl deleted the fix-hydra-plugin-discovery branch August 26, 2022 18:21
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