fix: binder runtime issues #1598

ppruthi · 2022-08-04T20:00:24Z

Related Issues/PRs

A few notebooks have runtime failures while running on mybinder.org. This PR fixes those issues.

What changes are proposed in this pull request?

Modifying jupyter notebooks in $REPO_ROOT/notebooks/features to run on mybinder.org as well as our test infrastrucuture.

How is this patch tested?

Our CI infra + manually running notebooks on mybinder.org

I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.

Does this PR change any dependencies?

No. You can skip this section.
Yes. Make sure the dependencies are resolved correctly, and list changes here.

Does this PR add a new feature? If so, have you added samples on website?

No. You can skip this section.
Yes. Make sure you have added samples following below steps.

Find the corresponding markdown file for your new feature in website/docs/documentation folder.
Make sure you choose the correct class estimators/transformers and namespace.
Follow the pattern in markdown file and add another section for your new API, including pyspark, scala (and .NET potentially) samples.
Make sure the DocTable points to correct API link.
Navigate to website folder, and run yarn run start to make sure the website renders correctly.
Don't forget to add  before each python code blocks to enable auto-tests for python samples.
Make sure the WebsiteSamplesTests job pass in the pipeline.

AB#1914413

github-actions · 2022-08-04T20:00:39Z

Hey @ppruthi 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.
We appreciate your patience and contributions 💯!

ppruthi · 2022-08-04T20:01:52Z

/azp run

azure-pipelines · 2022-08-04T20:02:05Z

Azure Pipelines successfully started running 1 pipeline(s).

codecov-commenter · 2022-08-04T20:38:49Z

Codecov Report

Merging #1598 (a5a5d89) into master (c960c06) will decrease coverage by 1.38%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1598      +/-   ##
==========================================
- Coverage   83.68%   82.30%   -1.39%     
==========================================
  Files         310      292      -18     
  Lines       15823    15473     -350     
  Branches      752      752              
==========================================
- Hits        13242    12735     -507     
- Misses       2581     2738     +157

Impacted Files	Coverage Δ
...soft/azure/synapse/ml/cognitive/AudioStreams.scala	`0.00% <0.00%> (-87.88%)`	⬇️
...t/azure/synapse/ml/cognitive/SpeechToTextSDK.scala	`18.03% <0.00%> (-72.55%)`	⬇️
...crosoft/azure/synapse/ml/cognitive/SpeechAPI.scala	`0.00% <0.00%> (-70.00%)`	⬇️
...osoft/azure/synapse/ml/param/TypedArrayParam.scala	`41.66% <0.00%> (-12.50%)`	⬇️
...ft/azure/synapse/ml/core/env/StreamUtilities.scala	`77.77% <0.00%> (-7.41%)`	⬇️
...a/com/microsoft/azure/synapse/ml/nn/BallTree.scala	`82.85% <0.00%> (-4.77%)`	⬇️
...rosoft/azure/synapse/ml/param/EstimatorParam.scala	`54.54% <0.00%> (-4.55%)`	⬇️
...om/microsoft/azure/synapse/ml/param/MapParam.scala	`72.72% <0.00%> (-4.55%)`	⬇️
...rosoft/azure/synapse/ml/param/DataFrameParam.scala	`60.71% <0.00%> (-3.58%)`	⬇️
...oft/azure/synapse/ml/param/UntypedArrayParam.scala	`59.37% <0.00%> (-3.13%)`	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ppruthi · 2022-08-04T23:20:03Z

/azp run

azure-pipelines · 2022-08-04T23:20:17Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-05T01:24:09Z

/azp run

azure-pipelines · 2022-08-05T01:24:23Z

Azure Pipelines successfully started running 1 pipeline(s).

mhamilton723 · 2022-08-05T02:23:54Z

notebooks/features/opencv/OpenCV - Pipeline Image Transformations.ipynb

+    "if platform == \"binder\":\n",
+    "    from IPython import get_ipython\n",
+    "\n",
+    "    !pip install matplotlib Pillow\n",


should we put these libraries in the binder image?

mhamilton723 · 2022-08-05T02:24:37Z

notebooks/features/classification/Classification - Adult Census.ipynb

+    "platform = current_platform()\n",
+    "if platform == \"synapse\":\n",


I know its a PITA but can we replace all of the places in the notebooks where we have this silly project arcadia thing?

Yep - that was the plan -- wanted to see if this works fine and does not regress on E2E tests

core/src/main/python/synapse/ml/core/utils/platform.py

mhamilton723 · 2022-08-05T13:30:36Z

core/src/main/python/synapse/ml/core/utils/platform.py

+        return "binder"
+    else:
+        return "unknown"
+


We might also want to add a findSecret API that will run through the gambit of looking in keyvaults, using databricks methods, and checking env vars before yelling and asking someone to replace that line with their key if they cant find anything

ppruthi · 2022-08-08T09:58:49Z

/azp run

azure-pipelines · 2022-08-08T09:59:03Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-08T15:57:04Z

/azp run

azure-pipelines · 2022-08-08T15:57:17Z

Azure Pipelines successfully started running 1 pipeline(s).

mhamilton723 · 2022-08-08T17:23:35Z

core/src/main/python/synapse/ml/core/utils/Platform.py

+def CurrentPlatform():
+    if os.environ.get("AZURE_SERVICE", None) == "Microsoft.ProjectArcadia":
+        return PLATFORM_SYNAPSE
+    elif "dbfs" in os.listdir("/"):
+        return PLATFORM_DATABRICKS
+    elif os.environ.get("BINDER_LAUNCH_HOST", None) is not None:
+        return PLATFORM_BINDER
+    else:
+        return PLATFORM_UNKNOWN
+
+
+def RunningOnSynapse():
+    if CurrentPlatform() is PLATFORM_SYNAPSE:
+        return True
+    return False
+
+
+def RunningOnBinder():
+    if CurrentPlatform() is PLATFORM_BINDER:
+        return True
+    return False
+
+
+def RunningOnDatabricks():
+    if CurrentPlatform() is PLATFORM_DATABRICKS:
+        return True
+    return False
+
+
+def PrintKeyWarning():


nit: use snake_case here

mhamilton723 · 2022-08-08T17:24:29Z

core/src/main/python/synapse/ml/core/utils/Platform.py

+
+
+def PrintKeyWarning():
+    if not RunningOnSynapse():


nit, throw an error here and refactor this to try to find a secret in a few places

ppruthi · 2022-08-08T19:07:42Z

/azp run

azure-pipelines · 2022-08-08T19:07:56Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-08T21:06:00Z

/azp run

azure-pipelines · 2022-08-08T21:06:14Z

Azure Pipelines successfully started running 1 pipeline(s).

mhamilton723 · 2022-08-08T21:37:16Z

core/src/main/python/synapse/ml/core/utils/Platform.py

+        raise RuntimeError(
+            "#### Please add your environment/service specific key(s) before running the notebook ####"
+        ) from None
+        return ""


Suggested change

raise RuntimeError(

"#### Please add your environment/service specific key(s) before running the notebook ####"

) from None

return ""

raise RuntimeError(f"Could not find {secret_name} in keyvault or overrides. If you are running this demo and would like to manually specify your key please add the override="YOUR_KEY_HERE" to the arguments of the find_secret method")

)

mhamilton723 · 2022-08-08T21:38:04Z

core/src/main/python/synapse/ml/core/utils/Platform.py

+    return current_platform() is PLATFORM_DATABRICKS
+
+
+def get_platform_specific_secret(searchKey):


Suggested change

def get_platform_specific_secret(searchKey):

def find_secret(secret_name, keyvault=SECRET_STORE, override=None):

mhamilton723 · 2022-08-08T21:38:32Z

core/src/main/python/synapse/ml/core/utils/Platform.py

+
+        spark = SparkSession.builder.getOrCreate()
+        dbutils = DBUtils(spark)
+        return dbutils.secrets.get(scope=SECRET_STORE, key=searchKey)


Suggested change

return dbutils.secrets.get(scope=SECRET_STORE, key=searchKey)

return dbutils.secrets.get(scope=keyvault, key=secret_name)

mhamilton723 · 2022-08-08T21:38:57Z

core/src/main/python/synapse/ml/core/utils/Platform.py

+    if running_on_synapse():
+        from notebookutils.mssparkutils.credentials import getSecret
+
+        return getSecret(SECRET_STORE, searchKey)


Suggested change

return getSecret(SECRET_STORE, searchKey)

return getSecret(keyvault, secret_name)

mhamilton723 · 2022-08-08T23:43:01Z

/azp run

azure-pipelines · 2022-08-08T23:43:14Z

Azure Pipelines successfully started running 1 pipeline(s).

mhamilton723 · 2022-08-08T23:46:00Z

/azp run

azure-pipelines · 2022-08-08T23:46:10Z

Azure Pipelines successfully started running 1 pipeline(s).

mhamilton723 · 2022-08-08T23:48:04Z

/azp run

azure-pipelines · 2022-08-08T23:48:18Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-09T01:00:43Z

/azp run

azure-pipelines · 2022-08-09T01:00:56Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-09T04:49:08Z

/azp run

azure-pipelines · 2022-08-09T04:49:23Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-09T06:16:26Z

/azp run

azure-pipelines · 2022-08-09T06:17:05Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-09T06:29:47Z

/azp run

azure-pipelines · 2022-08-09T06:30:01Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-09T08:25:17Z

/azp run

azure-pipelines · 2022-08-09T08:25:33Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi · 2022-08-09T16:00:32Z

/azp run

azure-pipelines · 2022-08-09T16:02:00Z

Azure Pipelines successfully started running 1 pipeline(s).

ppruthi requested review from svotaw and mhamilton723 as code owners August 4, 2022 20:00

ppruthi changed the title ~~fix binder runtime issues~~ fix: binder runtime issues Aug 4, 2022

mhamilton723 reviewed Aug 5, 2022

View reviewed changes

core/src/main/python/synapse/ml/core/utils/platform.py Outdated Show resolved Hide resolved

mhamilton723 reviewed Aug 5, 2022

View reviewed changes

ppruthi requested a review from eisber as a code owner August 8, 2022 09:57

mhamilton723 reviewed Aug 8, 2022

View reviewed changes

Puneet Pruthi added 3 commits August 8, 2022 19:42

fix bugs, test getsecret API

0722dd2

fix runtime errors

5b8e8b1

fix e2e failures

5f59669

mhamilton723 force-pushed the ms/ppruthi/fix-binder-run branch from 3122307 to c67cfb0 Compare August 8, 2022 23:42

chore: finish up cog service key fetching

20c3744

mhamilton723 force-pushed the ms/ppruthi/fix-binder-run branch from c67cfb0 to 20c3744 Compare August 8, 2022 23:46

chore: rename utils to platform package

cd8a890

chore: revert environment.yaml + fix notebook

9d0c706

chore: fix python test errors

e598bee

hammer fix

29d7faa

try updated ubuntu host

a5a5d89

mhamilton723 merged commit c7a61ec into microsoft:master Aug 9, 2022

		"platform = current_platform()\n",
		"if platform == \"synapse\":\n",

		return current_platform() is PLATFORM_DATABRICKS


		def get_platform_specific_secret(searchKey):

	def get_platform_specific_secret(searchKey):
	def find_secret(secret_name, keyvault=SECRET_STORE, override=None):

	return dbutils.secrets.get(scope=SECRET_STORE, key=searchKey)
	return dbutils.secrets.get(scope=keyvault, key=secret_name)

	return getSecret(SECRET_STORE, searchKey)
	return getSecret(keyvault, secret_name)

fix: binder runtime issues #1598

fix: binder runtime issues #1598

Conversation

ppruthi commented Aug 4, 2022 • edited Loading

Related Issues/PRs

A few notebooks have runtime failures while running on mybinder.org. This PR fixes those issues.

What changes are proposed in this pull request?

How is this patch tested?

Does this PR change any dependencies?

Does this PR add a new feature? If so, have you added samples on website?

github-actions bot commented Aug 4, 2022

ppruthi commented Aug 4, 2022

azure-pipelines bot commented Aug 4, 2022

codecov-commenter commented Aug 4, 2022 • edited Loading

Codecov Report

ppruthi commented Aug 4, 2022

azure-pipelines bot commented Aug 4, 2022

ppruthi commented Aug 5, 2022

azure-pipelines bot commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppruthi commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

ppruthi commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppruthi commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

ppruthi commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

mhamilton723 Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhamilton723 commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

mhamilton723 commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

mhamilton723 commented Aug 8, 2022

azure-pipelines bot commented Aug 8, 2022

ppruthi commented Aug 9, 2022

azure-pipelines bot commented Aug 9, 2022

ppruthi commented Aug 9, 2022

azure-pipelines bot commented Aug 9, 2022

ppruthi commented Aug 9, 2022

azure-pipelines bot commented Aug 9, 2022

ppruthi commented Aug 9, 2022

azure-pipelines bot commented Aug 9, 2022

ppruthi commented Aug 9, 2022

azure-pipelines bot commented Aug 9, 2022

ppruthi commented Aug 9, 2022

azure-pipelines bot commented Aug 9, 2022

ppruthi commented Aug 4, 2022 •

edited

Loading

codecov-commenter commented Aug 4, 2022 •

edited

Loading

mhamilton723 Aug 8, 2022 •

edited

Loading