Skip to content

Add implementation for listDownloadedModels. #2154

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 11, 2020
Merged

Add implementation for listDownloadedModels. #2154

merged 4 commits into from
Nov 11, 2020

Conversation

annzimmer
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes Override cla label Nov 6, 2020
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 6, 2020 •
edited
Loading

Coverage Report

Affected SDKs

  • firebase-ml-modeldownloader

    SDK overall coverage changed from 95.63% (4f3ffc5) to 93.65% (06ce22d8) by -1.97%.

    Filename Base (4f3ffc5) Head (06ce22d8) Diff
    CustomModel.java 89.29% 92.86% +3.57%
    SharedPreferencesUtil.java 98.80% 93.20% -5.59%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (06ce22d8) is created by Prow via merging commits: 4f3ffc5 f21799f.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 6, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • firebase-ml-modeldownloader

    Type Base (4f3ffc5) Head (06ce22d8) Diff
    aar 12.6 kB 14.3 kB +1.71 kB (+13.6%)
    apk (aggressive) 79.9 kB 80.1 kB +236 B (+0.3%)
    apk (release) 667 kB 668 kB +496 B (+0.1%)

Test Logs

Notes

Head commit (06ce22d8) is created by Prow via merging commits: 4f3ffc5 f21799f.

@annzimmer annzimmer requested review from vkryachko and rlazo November 6, 2020 14:53
@annzimmer annzimmer marked this pull request as ready for review November 6, 2020 14:54
rlazo
rlazo reviewed Nov 6, 2020
View reviewed changes
...downloader/src/main/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloader.java Outdated
@@ -84,7 +94,7 @@ public static FirebaseModelDownloader getInstance(@NonNull FirebaseApp app) {
/** @return The set of all models that are downloaded to this device. */
@NonNull
public Task<Set<CustomModel>> listDownloadedModels() {
throw new UnsupportedOperationException("Not yet implemented.");
return Tasks.forResult(sharedPreferencesUtil.listDownloadedModels());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since sharedPreferences are sync, you could try to create an actual task here to get concurrency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - thanks.

...der/src/main/java/com/google/firebase/ml/modeldownloader/internal/SharedPreferencesUtil.java
@@ -190,6 +196,24 @@ public synchronized void clearModelDetails(@NonNull String modelName, boolean cl
.commit();
}

public synchronized Set<CustomModel> listDownloadedModels() {
Set<CustomModel> customModels = new HashSet<>();
Set<String> keySet = getSharedPreferences().getAll().keySet();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're reading all the keys at once and not dealing with the sharedpreferences anymore, it may not be necessary to sync the whole method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second part (added todo) will need to coordinate with android download manager, so I'll need the sync when I add that.

rlazo
rlazo reviewed Nov 10, 2020
View reviewed changes
...der/src/main/java/com/google/firebase/ml/modeldownloader/internal/SharedPreferencesUtil.java Outdated
}
}
return customModels;
}

synchronized CustomModel isDownloadCompleted(String modelName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the is prefix in the name suggests a boolean response. What about using maybe, like maybeGetUpdatedModel? It's a bit more verbose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...loader/src/test/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloaderTest.java Outdated
FirebaseApp.clearInstancesForTest();
// default app
FirebaseApp.initializeApp(ApplicationProvider.getApplicationContext(), FIREBASE_OPTIONS);
firebaseModelDownloader = new FirebaseModelDownloader(FIREBASE_OPTIONS, mockPrefs);

executor = new ThreadPoolExecutor(0, 1, 30L, TimeUnit.SECONDS, new LinkedBlockingQueue<>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use a singleThread executor like in

firebase-android-sdk/firebase-messaging/src/test/java/com/google/firebase/messaging/ImageDownloadRoboTest.java

Line 53 in fe51be9

executor = Executors.newSingleThreadExecutor();

That way the test shouldn't finish before running the tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

rlazo
rlazo reviewed Nov 11, 2020
View reviewed changes
...loader/src/test/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloaderTest.java Outdated

@After
public void cleanUp() {
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK this is no longer needed when using the single thread executor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - going too fast.

...loader/src/test/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloaderTest.java Outdated
assertThat(task.isComplete()).isTrue();
assertEquals(customModelSet, Collections.singleton(CUSTOM_MODEL));

executor.awaitTermination(500, TimeUnit.MILLISECONDS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably get rid of this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

vkryachko
vkryachko reviewed Nov 11, 2020
View reviewed changes
...loader/src/test/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloaderTest.java
TestOnCompleteListener<Set<CustomModel>> onCompleteListener = new TestOnCompleteListener<>();
Task<Set<CustomModel>> task = firebaseModelDownloader.listDownloadedModels();
task.addOnCompleteListener(executor, onCompleteListener);
Set<CustomModel> customModelSet = onCompleteListener.await();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if Tasks.await(task) could be used instead here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tried that got - "java.lang.IllegalStateException: Must not be called on the main application thread", found this solution was used elsewhere.

@google-oss-bot
Copy link
Contributor

@annzimmer: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
smoke-tests f21799f link /test smoke-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@annzimmer
Copy link
Contributor Author

smoke test keeps failing but I don't think it's related to my change.

@rlazo
Copy link
Collaborator

rlazo commented Nov 11, 2020

smoke test keeps failing but I don't think it's related to my change.

It's OK to ignore for now, it's a bug in the tests themselves.

@annzimmer annzimmer merged commit 4eca657 into master Nov 11, 2020
@annzimmer annzimmer deleted the listModels branch November 16, 2020 20:08
@firebase firebase locked and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vkryachko vkryachko vkryachko left review comments

@rlazo rlazo rlazo approved these changes

Assignees

@pavelgj pavelgj

@manjanac manjanac

@annzimmer annzimmer

Labels
cla: yes Override cla size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@annzimmer @google-oss-bot @rlazo @vkryachko @pavelgj @googlebot @manjanac