dagster_azure: do not import fake implementations by default

[futurwasfree]

Related to issue #26733

Summary & Motivation

Currently when I import the following symbols in my production setup:

from dagster_azure.adls2.io_manager import adls2_pickle_io_manager
from dagster_azure.adls2.resources import adls2_resource

I'm also getting all the fake_adls2_resources imported, as it's described in adls2/init.py:

from dagster_azure.adls2.fake_adls2_resource import (
    FakeADLS2Resource as FakeADLS2Resource,
    FakeADLS2ServiceClient as FakeADLS2ServiceClient,
    fake_adls2_resource as fake_adls2_resource,
This easily adds 0.3-0.5 seconds to overall import/startup time.

How I Tested These Changes

python -X importtime definitions.py
before and after the change

Changelog

Insert changelog entry or delete this section.

[ion-elgreco]

@dpeng817 Could you take a look at this pr?

[dpeng817]

While I agree that these should probably exist in a separate namespace, it's unfortunately a breaking change. These are documented as public in the API docs under this namespace, and therefore it's not a change I think we can make in good conscience unfortunately.

Curious why this is coming up as an issue for you. Is there a package or something which needs to be installed which is causing problems?

[futurwasfree]

No, as I've mentioned in #26733 it adds a bit more to startup time of each pod where you have to import definitions of ADLS2 including fakes (which I'm not interested in prod deployment).
Happy to update docs or discuss any other way to address it.
While it might be a breaking change, you always need to evaluate if many people are actually using it. And how many of them will benefit from this change.

[dpeng817]

Okay @futurwasfree ; after discussing internally I think since this isn't a 1.0 library, and because we believe these APIs to be relatively low traffic, we're okay doing a breaking change in the next .0 release (1.10.0).

What we need to do is the following:

• Add a new dagster_azure.fakes submodule where we can include these fakes instead.
• Issue a deprecation warning on imports of the code from the original path

Then, in 1.10.0, we can actually delete the original imports.

In terms of issuing a warning, I think what you want to do is move the fakes to dagster_azure.fakes under a new name, and then under the original name subclass the fake. In the init of that fake, issue the deprecation warning.

cc @maximearmstrong for future coordination here.

[futurwasfree]

Thanks @dpeng817 for looking into it and explanations.
Just want to check if I understood it correctly: after all the changes, won't you get deprecation warning on any import from dagster_azure? since fakes are currently imported in central init.py ? Is that what we want, just double checking?

[futurwasfree]

I've re-read your message and did something ;) could you take a look please?

[dpeng817]

This looks good, I'll merge when tests pass. Thanks!

[futurwasfree]

You're very welcome!
I'm not sure about test failure here, seems unrelated? Should I rebase maybe?

[dpeng817]

I'll fix test failures post-hoc @futurwasfree ; thanks again for your contribution.

[futurwasfree]

Great stuff @dpeng817, I'm really happy that you've accepted my changes!

One last question maybe about 2nd act of our play, i.e. removal of old submodule prior to next minor release (1.10.0): will you ping me later to do it? or will you guys handle it on your own?

[dpeng817]

@futurwasfree we'll handle that part on our own, unless you're particularly interested in doing it!

[futurwasfree]

@dpeng817 nope, I trust you 100% on this!