Potential Issues Discussion for Technical Team

[msheller]

Hey @aristizabal95 @prakashngit @johnugeorge @hasan7n ,

Capture questions/issues you run into while using medperf here so we can figure out which need to be filed as issues.

Thanks!
Micah

[aristizabal95]

It should be possible to test the workflows (data prep, benchmark execution) independently. This allows more granular testing, like checking the implementation of a data prep cube or model-results cubes without having the full pipeline ready.

Specific case: @msheller wanted to test an implementation of a data prep cube inside medperf, but it is not possible at the moment without the rest of the workflow.

[aristizabal95]

Moving contents of #79 here, as it seems to fit the discussion

This issue is to capture the set of questions we want to follow up with the alpha testers on as the alpha progresses.

1. Authentication sources to support (e.g. Github, Google, etc...) in beta
2. MLCube pipelines vs. single containers
3. Manual steps in the middle of preparation stages. Run as multiple stages that medperf comprehends, or run as "pauses" in the preparation cube?
4. Hosting public datasets in cloud buckets?

[aristizabal95]

Out of band files:

We need to support some form of out of band files that can't be directly downloaded. The currently proposed method is that:

The file names/paths and hashes are still registered so that we can ensure integrity of the algorithm.
The medperf client checks for all files, downloads what it can, and if anything is still missing, the client informs the user about the missing files and exits. For example, "/private_weights.tar could not be downloaded automatically. This may be intentional (e.g. a file that requires a signed agreement to download). Please get this file through appropriate means and re-run the command" or similar. Something to let them know that this may well be intended.

[aristizabal95]

The name "demo dataset" seems to be confusing

[aristizabal95]

Sample dataset could be a more appropiate name

[hasan7n]

ISSUE CREATED

I understood from the code that mlcube.yaml and parameters.yaml are expected to be hosted on github? (starting with "https://raw.githubusercontent.com"). If yes, why?

[aristizabal95]

This is because we can leverage hosting from github, as well as hash checking. We require it to be in "https://raw.githubusercontent.com" so that calling this url with requests returns the contents of the file, instead of a website with html/css/js. We also want the link to include the commit hash so that integrity is ensured (updating the repo won't change the contents of that particular commit). A better check would be ideal so we can ensure people use the commit hash, but for now this has not been seen as important. We can create an issue for that though.

[hasan7n]

ISSUE CREATED

The command dataset create takes a benchmark as a parameter only to use its data preparation cube. I think it is better to be a preparation cube only.I am imagining the following scenario:

say there is a benchmark out there, doing fine and having many collaborators. I am a new data owner but for some reason (fresh data maybe) my raw data is not compatible with the benchmark data preparation cube. (e.g. different image formats). I know a developer that is willing to create a prep cube for my data. Currently, there is no way for me to join the benchmark.

If the above scenario makes sense, we should also remove a condition for running benchmarks. Currently, a benchmark execution is rejected if the dataset used was prepared by a prep cube different from the benchmark's. It seems removing this condition isn't harmful (correct me here if I am wrong), because executing a benchmark takes into account only the outputs, and as long as these outputs comply with the model-evaluator flow by the association condition, we are fine.

This also may give a motive to think about allowing benchmarks to have a reference prep cube and other added, 'on-demand' prep cubes.

[hasan7n]

Micah added:

• prepending prepare steps and appending sanity checks: this can ensure flexibility of preparation, and doesn't break the 'contract' with the model owner.

[aristizabal95]

This has been covered by #227. Thanks Hasan!

[hasan7n]

ISSUE CREATED

A few very rare edge cases to keep in mind regarding data submission:

1. The scenario of having different users preparing and submitting the same dataset (e.g. somebody wants to test the platform and used some public datasets)
2. Different data input hashes resulting in the same generated prepared data uid. (e.g. a user preparing their dataset with a different labels format and different prep cube, and unaware that the same dataset will be generated)

[aristizabal95]

1. The reason we have this is because we want to avoid dataset collision as much as possible. If the issue is regarding the UX, or what the user thinks broke because of his submission attempt, then maybe we should be more verbose and describe better what happened, and why that is not allowed. (e.g. "The dataset you tried to submit has already been submitted by another user. We currently don't allow duplicate datasets on the server")
2. Same thing, we can be more verbose, and describe possible reasons why the user might end up with the same generated_prepared data uid. (e.g. This prepared dataset already exists. This could either be because the same input dataset is being used, or the data preparation cube might result in the same output for different structures")

[aristizabal95]

These can be simple issues we can work on

[hasan7n]

In case we found it is common for users to host files (weights, params, ...) on google drive, we might add a feature to make medperf use gdown instead of wget when needed.

from stackoverflow:

pip install gdown
After that, you can download any file from Google Drive by running one of these commands:

gdown https://drive.google.com/uc?id=<file_id>

The file_id should look something like 0Bz8a_Dbh9QhbNU3SGlFaDg. You can find this ID by right-clicking on the file of interest, and >selecting Get link. As of November 2021, this link will be of the form:

https://drive.google.com/file/d/<file_id>/view?usp=sharing

[hasan7n]

ISSUE CREATED

workspace/additional_files is hardcoded in config; cube authors must know that they should use 'workspace/additional_files" as the path of the folder containing their additional files.

[aristizabal95]

This requires better documentation

[hasan7n]

ISSUE CREATED

medperf/cli/medperf/comms/rest.py

Lines 346 to 347 in c90e7e9

	def __get_cube_file(self, url: str, cube_uid: int, path: str, filename: str):
	res = requests.get(url)

I am wondering, in case we encountered very large additional files links, will this be loaded in RAM?

[aristizabal95]

This is totally right! Very sharp observation. We can use this code to make file retrieval use streaming procedures to restrict memory usage while downloading files. This is a good issue to work on

[hasan7n]

ISSUE CREATED

If we are expecting users to run simultaneously multiple medperf commands, we should consider the following scenario:

command A: a certain benchmark execution
command B: preparation of some dataset

commands A and B are being run simultaneously.

if command A fails for some reason, cleanup will be called by default, erasing all folders starting with tmp_, probably including a folder currently being using by command B.

Should test to confirm.

[aristizabal95]

Good point. We can get the python execution PID as described here and append it to the tmp suffix at the beginning of every execution. This way, when we reach the cleanup procedure, it will by default only clean up things that are named with tmp_{PID}_.

We can create an issue for this

[hasan7n]

Issues regarding the examples folder:

1. we modified the examples in our folder so that the mlcube.py file doesn't use subprocess. The docker images should also be updated if they aren't. Medperf uses the images, not the code. Warning: Chexpert is used in our integration tests.
2. DFCI experiments docker images are not yet on dockerhub.
3. .so files are ignored in .gitignore. DFCI example uses a .so file which is currently missing and hence using the code will throw bugs.

[hasan7n]

1. refactoring get like how all is refactored (_remote_get, _local_get)
2. sharing code across entities (inheritence, mixins)
3. compatibility test caching will not be valid because of benchmark tmp uid is dependant on the process id. Introducing the notion of unique tmp uid might resolve this (to check)
4. Check if we need to call the benchmark_models_list when executing the benchmark, as this is no longer called in bmk.all (to check)

[hasan7n]

ISSUE CREATED

about the ignore_errors option for running a benchmark experiment:
It ignores the mlcube failure and hence the client would upload partial results.
The current logic ignores the errors of both the model cube and the metrics cube, which sounds to not make sense. A failed metrics mlcube will not generate the results file at all. To check

[hasan7n]

config.ui and config.comms should be exclusive for UI and COMMS classes. better to define additional config.ui_interface and config.comms_interface

[hasan7n]

medperf storage is enforced to be at $HOME. Is this an issue? Users might have their machines rely on external disks for storage.

[hasan7n]

Possible solution:
prompt the user at installation or at their first run to specify where they want the storage to be. any downsides?

[hasan7n]

benchmark submission without a demo dset: do we really have to allow this? Our documentation mentions steps to follow if a benchmark owner can't host a demo dset: link

something doesn't sound right :) (we are asking the benchmark owner to send their demo dset to hospitals and ask them for manual steps, ...)
I think ideally we should require hosting a demo dataset, or think about providing hosting solutions?

[hasan7n]

To keep in mind: currently our code interface accepts a submission without a demo dataset but fails when trying to run the compatibility test

[hasan7n]

closing this to keep track of such issues in a unified place