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

Thoughts on turning adafruit_bitmap_font / adafruit_fakerequests into a soft dependency? #55

Open
joshkehn opened this issue Nov 7, 2021 · 8 comments

Comments

@joshkehn
Copy link

joshkehn commented Nov 7, 2021

It looks like adafruit_bitmap_font (repo) is used as a convenience function to _load_font:

if font not in self._fonts:
self._fonts[font] = bitmap_font.load_font(font)

This could be turned into an inline import. For example:

 if font not in self._fonts:
    from adafruit_bitmap_font import bitmap_font  # <-- Inline import 
    self._fonts[font] = bitmap_font.load_font(font)

Turning this into an inline import would make this a soft dependency. If it's not used it's not required. I was working up a small demo on the MagTag but not using anything about the display or Wi-Fi. I noticed that I still needed quite a few libraries. Some of these, like adafruit_fakerequests, aren't used even if I was using the full capabilities on the device. For example, this line could also use an inline import:

response = Fake_Requests(LOCALFILE)

Making these inline imports would reduce the number of hard dependencies here. This would also provide some space savings and improve the speed at which the code starts up. This also shouldn't functionally impact any existing example code which makes use of functions requiring these libraries. They would still require the library to be installed.

Is this worth submitting a PR for?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Nov 8, 2021

One thing to investigate about this would be how the project bundling system would treat it. AFAIK there is something scanning the imports at the top of the file to determine which libraries need to get bundled with a project. If this import isn't there any more it may not include it in the bundle which could then confuse folks working on projects that do use this functionality if they didn't get that library in their download bundle zip.

If this does end up working how I am thinking maybe a work around would be wrapping the import in try/except for the ImportError if it's missing. Then it should get included in the bundle by default, but advanced users could delete it if they know that their project isn't using that functionality.

@joshkehn
Copy link
Author

joshkehn commented Nov 8, 2021

@FoamyGuy I didn't know there was a project bundling system! I've been manually copying dependencies to lib/. Do you have a link to this where I can investigate more how it works?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Nov 8, 2021

@joshkehn this video discusses it a bit: https://www.youtube.com/watch?v=UgekT8epJjo

I'm not sure if there is much information available about how the bundling works though. I would encourage you to join the Adafruit discord and ask around in the #circuitpython-dev channel. Or if your available for it in a few hours today (2pm Eastern Time) we are having our weekly meeting. At the end of it we always have an "in the weeds" section were folks can ask or dive deeper into topics that need or benefit from a wider discussion with the group. You could add a note asking about it int he notes doc to get it discussed.

That being said, I did work on the screenshot utility that is mentioned in the latter half of this video. I think maybe it works similarly to the bundling tool. The screenshots use this project https://github.com/mgedmin/findimports to find which libraries are used.

To start with it'd probably be good to run this against a branch of this repo modified with the changes you propose and see whether moving the import to inline causes it to get missed by this. If so then we could look into work-arounds.

@makermelissa
Copy link
Contributor

@jwcooper do you think this type of change would still work with the Learn bundling system?

@jwcooper
Copy link
Member

jwcooper commented Dec 1, 2021

We just need to make sure that findimports (https://pypi.org/project/findimports/) can find the inline imports. It's easy enough to test this on the CLI, I think it's just findimports {file_name}.

@jwcooper
Copy link
Member

jwcooper commented Dec 1, 2021

Sorry, a correction...this should be OK, because we just scan the original example/code files, not the library files with findimports. We use the requirements.txt file to determine library dependencies, so as long as all optional dependencies remain in the requirements.txt, we should be OK.

@makermelissa
Copy link
Contributor

Excellent, thank you.

@tekktrik
Copy link
Member

It is worth mentioning that optional requirements are now stored in optional_requirements.txt. Not sure if that is currently handled, but thought it relevant to this issue.

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

No branches or pull requests

5 participants