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

clarify caveats in README #3

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timotheecour
Copy link
Collaborator

@timotheecour timotheecour commented Jan 9, 2024

Pre-existing docs were misleading in several ways (see below), this PR clarifies the README and also shows caveats with using monorepo.load_package (in particular that it breaks modularity, can cause module clashes and duplicate modules that can result in silent runtime bugs, see PR README).

IMO monorepo.load_package should be deprecated and this PR suggests simple and standard alternatives to achieve same goals (using uniform import syntax from anywhere in monorepo) by simply adding monorepo root to PYTHONPATH (either programmatically or on command line).

  • foo.bar_function() in the README gave the impression it would work for modules but it only works for packages (eg if bar_function() is in foo/init.py)
  • foo = monorepo.load_package("example.package.foo") was mis-leading as the returned name is not used, eg:
monorepo.load_package("example.package.foo")
from foo import mymodule1 # ok
foo = monorepo.load_package("example.package.foo")
from foo import mymodule1 # ok
foo2 = monorepo.load_package("example.package.foo")
from foo2 import mymodule1 # error: foo2 not defined

There is code in the wild that falls into this trap, eg starfo*/read_data_test.py: char_dataset = monorepo.load_package("common.dataset.dataset")

@timotheecour timotheecour changed the title monorepo.load_package considered harmful? monorepo.load_package: clarify caveats in README Jan 9, 2024
@timotheecour timotheecour changed the title monorepo.load_package: clarify caveats in README clarify caveats in README Jan 9, 2024
@timotheecour timotheecour marked this pull request as draft May 30, 2024 01:28
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.

1 participant