metaflow icon indicating copy to clipboard operation
metaflow copied to clipboard

Request: Allow shadowing in metaflow_extensions

Open JettJones opened this issue 1 year ago • 2 comments

This is kind of an elaborate use case, but my team is frequently running into issues while using metaflow_extensions where our internal definitions are found multiple times, causing the program to fail with the Conflicts RuntimeError while checking for shadowed extensions:

https://github.com/Netflix/metaflow/blob/master/metaflow/extension_support/init.py#L761

The path where we'd hit this error is:

  1. Running on an external platform, we install our source as a python package.
  2. Then, we also copy a subset of our source, and place it earlier in sys.path - this gets us ready to live edit some sources, while having all dependencies (and third party code) to fall back on.
  3. But then at runtime - an import to metaflow sources triggers the extension logic init - and two copies of metaflow_extensions raises an error.

I'd like to be able to tell metaflow (either in general or for our extension path ), that the conflict is ok and expected - maybe a METAFLOW_EXTENSIONS_BUT_CHILL environment variable, that turns the error into a warning. Stated differently - the python path is defined correctly in our case, the version that is found first is the one we intend to use, so the extra check in extension_support is all that blocks our workflow.

I also may be missing some use case for _AliasLoader so if there's an easier work-around I'm excited to take pointers. Thanks!

JettJones avatar Aug 20 '24 18:08 JettJones

Hey @JettJones -- thanks for the feedback. Let me make sure I understand properly:

  • you package all your source (including metaflow and extensions) in a python package
  • you install that package
  • you also copy a portion of your code (including extensions) earlier in your sys.path to effectively "shadow" the code that was installed by the python package?

Is that correct?

If so, yes, you are right that the current extensions mechanism will complain when it can't figure out what extensions you are trying to import. The rationale behind it is that the metaflow_extensions package is actually a namespace package and it therefore looks for all "participants" to this namespace package.

Could you let me know what exact error message is being printed by the line you point to? Depending on that, there may be a way of doing it by, for example, ignoring packages that are "in the path" and only looking at installed packages (although I don't know if that is what you want -- it would remove the ability to "edit" the files that you put earlier in your path).

We could also add a flag as you mention but am a bit weary of adding something that explicitly breaks functionality. We could call it METAFLOW_I_KNOW_THE_RISKS_DISABLE_EXTENSIONS_CHECK or something like that but would rather scope it as narrowly as possible. The loading mechanism for extensions is already a bit complicated and want to avoid making it even more obscure.

And as a total sidenote, it's METAFLOW_EXTENSIONS_AND_CHILL :).

romain-intel avatar Aug 21 '24 07:08 romain-intel

Thanks @romain-intel - you're right about the setup - just those three steps.

Could you let me know what exact error message is being printed by the line you point to? Sure, we typically see this, plus a stack trace starting from import metaflow somewhere in our libraries.

RuntimeError: Conflicts in 'metaflow_extensions' files:
    Packages 'company-bundle[comp]', and '/users/jett/git-checkout/libraries/python/metaflow_extensions[comp]' define the same configuration module 'metaflow_extensions.comp.config.__init__'

I think defaulting to installed packages (and ignoring source paths) could work - it's less ideal for our specific case, because we're piecing together live editing where the sources should be the first choice. Less ideal but maybe acceptable, because we're usually editing sources other than metaflow_extensions, so getting away from the RuntimeError would unblock us in the 99% case. (And if we ever wanted to live edit extensions, we'd find another path)

And to read back my understanding of namespace packages - because python would be just as happy loading a standard package named metaflow_extensions, most of the error checks in extension_support/__init__.py are flagging what look like user errors - setups that can hide other metaflow_extensions either the whole package, or individual sub-folders with the same name. I think I'm coming from the perspective where functionality (python loading modules) is broken by additional checks, but given the ability to protect from user error and confusion, I totally follow what extension_support wants to flag via exceptions here.

would rather scope it as narrowly as possible.

Definitely - I'm coming in with a 30 minute understanding of the code here, so I'm happy to follow other folk's definition of what would be narrow or easy in this case.

And as a total sidenote, it's METAFLOW_EXTENSIONS_AND_CHILL :).

:laughing:

JettJones avatar Aug 21 '24 16:08 JettJones

Interestingly, I am trying to solve another problem that would require me to "restrict" where the extension mechanism looks for participants. I thought of this at the same time and wonder if I can solve two issues with one "fix". Would something like the following satisfy your use case:

  • provide an env variable that would be a list of paths (so something like /a/b/c;/d/e/f...)
  • that would instruct metaflow to only look in those directories for extensions (whether they come from python path or from distributions). In other words, if you want to include just the ones from a specific python path, you would include just that path, if you want to just use the ones that are installed on the system, you could list the path of your distribution (ie: the site-packages directory).

romain-intel avatar Sep 04 '24 01:09 romain-intel

Here is the PR #2018. It's not fully wired up but here https://github.com/Netflix/metaflow/pull/2018/files#diff-2716376d206903ab020208c72913214109deb6b92f21db6dbb2bc9bfb92eef4aR863 you could possibly inject an env-var to set the restrict_to_directories argument that I added here: https://github.com/Netflix/metaflow/pull/2018/files#diff-2716376d206903ab020208c72913214109deb6b92f21db6dbb2bc9bfb92eef4aR334. If that works for you, I can add that as well.

romain-intel avatar Sep 05 '24 08:09 romain-intel

Yes, I think all our use cases could know or look up the local source paths ahead of loading metaflow, so that workaround should be enough for this bug.

JettJones avatar Sep 06 '24 21:09 JettJones