Page MenuHomeElementl

sidkmenon (Sid Menon)
Disabled

Projects

User does not belong to any projects.

User Details

User Since
Jan 19 2021, 9:41 PM (39 w, 2 d)
Roles
Disabled

Recent Activity

Jul 28 2021

sidkmenon requested review of D9116: [Easy] Adding serialization decorators & convenience methods.
Jul 28 2021, 9:39 AM

Jul 15 2021

sidkmenon accepted D8909: switch from using String to Text to match schema for mode col.
Jul 15 2021, 7:10 PM

Jul 13 2021

sidkmenon added inline comments to D8841: [docs] Dagster K8s deployment guide corrections - GH Issue #4234.
Jul 13 2021, 7:36 PM
sidkmenon updated the summary of D8841: [docs] Dagster K8s deployment guide corrections - GH Issue #4234.
Jul 13 2021, 5:12 PM
sidkmenon updated the diff for D8841: [docs] Dagster K8s deployment guide corrections - GH Issue #4234.

Updating to address @rexledesma's comments

Jul 13 2021, 5:11 PM

Jul 12 2021

sidkmenon updated the summary of D8841: [docs] Dagster K8s deployment guide corrections - GH Issue #4234.
Jul 12 2021, 10:32 PM
sidkmenon updated the summary of D8841: [docs] Dagster K8s deployment guide corrections - GH Issue #4234.
Jul 12 2021, 10:32 PM
sidkmenon requested review of D8841: [docs] Dagster K8s deployment guide corrections - GH Issue #4234.
Jul 12 2021, 10:31 PM

Jul 9 2021

sidkmenon accepted D8797: refactor(helm): remove enabled flag in run coordinator.

lgtm besides the comment

Jul 9 2021, 3:00 PM

Jul 8 2021

sidkmenon abandoned D6810: [core][dagster-mysql] Fixing @experimental tag to work for classes.
Jul 8 2021, 1:33 PM

Jul 2 2021

sidkmenon accepted D8683: fix docs build + remove rc versions + add nav json for 0.11.16.

lgtm

Jul 2 2021, 6:58 PM
sidkmenon accepted D8672: Unpin urllib3 in automation.
Jul 2 2021, 3:15 PM

Jun 30 2021

sidkmenon accepted D8634: chore: implement str representation for serializable error info.

Nice!

Jun 30 2021, 8:20 PM

Jun 29 2021

sidkmenon accepted D8586: docs: fix graphql query to return failed runs.
Jun 29 2021, 2:44 AM

Jun 21 2021

sidkmenon accepted D8483: test: add retries to individual tests in helm template suite.

Lgtm! Nice find w/ pytest-rerun

Jun 21 2021, 4:43 PM

Jun 18 2021

sidkmenon added inline comments to D8439: [helm] Add k8s fields to pydantic schema.
Jun 18 2021, 8:27 PM
sidkmenon updated the diff for D8439: [helm] Add k8s fields to pydantic schema.

Based on discussion with @rexledesma, refactoring to just use Container as a schema

Jun 18 2021, 8:18 PM
sidkmenon updated the diff for D8439: [helm] Add k8s fields to pydantic schema.

Allowing for imagePullPolicy as well

Jun 18 2021, 7:21 PM
sidkmenon accepted D8469: [dagit] Remove session-token header.

Sounds good to me! This would break run attribution in the demo, right, since that tries to read the header (as a proof of concept for the prezi ask, I think?)

Jun 18 2021, 7:07 PM
sidkmenon added inline comments to D8439: [helm] Add k8s fields to pydantic schema.
Jun 18 2021, 6:42 PM
sidkmenon updated the diff for D8439: [helm] Add k8s fields to pydantic schema.

Addressing 'TODO'

Jun 18 2021, 5:21 PM
sidkmenon added a reviewer for D8439: [helm] Add k8s fields to pydantic schema: jordansanders.
Jun 18 2021, 3:24 PM
sidkmenon added inline comments to D8439: [helm] Add k8s fields to pydantic schema.
Jun 18 2021, 7:46 AM
sidkmenon updated the diff for D8439: [helm] Add k8s fields to pydantic schema.

responding to @rexledesma's comments

Jun 18 2021, 7:45 AM

Jun 17 2021

sidkmenon added inline comments to D8441: [helm][schema] Make HelmTemplate more generic.
Jun 17 2021, 9:29 PM
sidkmenon updated the diff for D8441: [helm][schema] Make HelmTemplate more generic.

requiring config of helm_dir_path & subchart_paths via @rexledesma's comments

Jun 17 2021, 9:26 PM
sidkmenon updated the diff for D8439: [helm] Add k8s fields to pydantic schema.

Also increase number of retires on helm checks since that appears to be a problem

Jun 17 2021, 9:12 PM
sidkmenon published D8439: [helm] Add k8s fields to pydantic schema for review.
Jun 17 2021, 9:09 PM
sidkmenon published D8441: [helm][schema] Make HelmTemplate more generic for review.
Jun 17 2021, 8:38 PM

Jun 10 2021

sidkmenon added a reviewer for D8330: [dagster-graphql-client] Make `use_https` required: owen.
Jun 10 2021, 4:46 PM
sidkmenon requested review of D8330: [dagster-graphql-client] Make `use_https` required.
Jun 10 2021, 4:14 PM

Jun 9 2021

sidkmenon added inline comments to D8320: [dagster-graphql-client] Allow HTTPS use.
Jun 9 2021, 11:19 PM
sidkmenon requested review of D8320: [dagster-graphql-client] Allow HTTPS use.
Jun 9 2021, 10:02 PM
sidkmenon updated the diff for D8313: [dagster-graphql-client] Fixing Exception messages.

more docstring edits

Jun 9 2021, 9:19 PM
sidkmenon requested review of D8317: [dagster-graphql-client] Fix `get_run_status` return type.
Jun 9 2021, 8:38 PM
sidkmenon updated the diff for D8313: [dagster-graphql-client] Fixing Exception messages.

Fix docstrings

Jun 9 2021, 8:17 PM
sidkmenon requested review of D8313: [dagster-graphql-client] Fixing Exception messages.
Jun 9 2021, 6:58 PM

Jun 7 2021

sidkmenon accepted D8274: Add test that runs graphql_client.get_run_status against a real graphql context.

This looks fine to me. I'll take a look at that Drizly problem ASAP.

Jun 7 2021, 7:40 PM
sidkmenon accepted D8268: Fixes for dagster-mysql local runs.

lgtm! Haven't run into the Interface error personally but I figure if it's working locally for you it's fine!

Jun 7 2021, 4:33 PM

Jun 3 2021

sidkmenon updated the diff for D8206: [docs] Run Attribution.

responding to @rexledesma's comments

Jun 3 2021, 6:46 PM
sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

Removing patch on warnings

Jun 3 2021, 6:29 PM
sidkmenon updated the summary of D8206: [docs] Run Attribution.
Jun 3 2021, 4:08 PM
sidkmenon updated the summary of D8206: [docs] Run Attribution.
Jun 3 2021, 3:43 PM
sidkmenon published D8206: [docs] Run Attribution for review.
Jun 3 2021, 3:42 PM

Jun 2 2021

sidkmenon added a comment to D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

This example needs to show up in the docs somewhere, probably in https://docs.dagster.io/deployment/run-coordinator. Similar to https://docs.dagster.io/deployment/guides/docker#example, we should highlight the fact that a custom run coordinator can be used, and that an example is a run coordinator that injects run tags

Jun 2 2021, 8:34 PM
sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

rebasing & prefixing test class with Test** to adhere to pytest conventions

Jun 2 2021, 5:50 PM
sidkmenon updated the diff for D8187: [dagster][core] Adding shared QueuedRunCoordinator test suite.

prefixing with Test to adhere to pytest conventions

Jun 2 2021, 5:49 PM

Jun 1 2021

sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

rebasing

Jun 1 2021, 8:51 PM
sidkmenon updated the diff for D8187: [dagster][core] Adding shared QueuedRunCoordinator test suite.

Renaming to QueuedRunCoordinatorTests as per @jordansanders' advice

Jun 1 2021, 8:42 PM
sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

Fix tox

Jun 1 2021, 8:37 PM
sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

Fix import

Jun 1 2021, 8:36 PM
sidkmenon requested review of D8187: [dagster][core] Adding shared QueuedRunCoordinator test suite.
Jun 1 2021, 8:22 PM
sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

rename run_attribution -> run_attribution_example

Jun 1 2021, 8:12 PM
sidkmenon updated the diff for D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.

restructuring to address @rexledesma's comments as well as rebasing

Jun 1 2021, 8:05 PM
sidkmenon updated the summary of D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator.
Jun 1 2021, 7:59 PM
sidkmenon abandoned D8036: [core][tests] Shared RunCoordinator test suite (2/n).
Jun 1 2021, 7:58 PM
sidkmenon published D8043: [examples] Adding a Run Attribution Example via a Custom Run Coordinator for review.
Jun 1 2021, 5:30 PM
sidkmenon accepted D8181: remove gh issue for cursor_res.
Jun 1 2021, 5:29 PM
sidkmenon added a comment to D8181: remove gh issue for cursor_res.

This issue was there to track a mypy problem because we had a pin in SqlAlchemy for <1.4 (I think related to this diff). I'll add in the type annotation in a separate diff - this LGTM. thanks for catching this Alex.

Jun 1 2021, 5:29 PM
sidkmenon accepted D8176: Fix mysql docs event log module.

Oops, looks like I missed this - I'll work on getting these under test ASAP.

Jun 1 2021, 3:36 PM
sidkmenon published D8036: [core][tests] Shared RunCoordinator test suite (2/n) for review.
Jun 1 2021, 3:33 PM
sidkmenon published D8030: [helm] Adding configurable RunCoordinator (1/n) for review.
Jun 1 2021, 3:33 PM

May 20 2021

sidkmenon updated the diff for D8020: [dagster-mysql] `wait_timeout` fix for MySQL Engine Connections.

Adding in asserts & cleaning up tests

May 20 2021, 9:08 PM
sidkmenon updated the diff for D8020: [dagster-mysql] `wait_timeout` fix for MySQL Engine Connections.

Added tests & removed the NullPool wait_timeout fix - there's no pool for that case so there's nothing to recycle

May 20 2021, 9:01 PM
sidkmenon requested review of D8020: [dagster-mysql] `wait_timeout` fix for MySQL Engine Connections.
May 20 2021, 8:10 PM

May 19 2021

sidkmenon accepted D7975: Fix permalinks.
May 19 2021, 12:48 AM
sidkmenon published D7975: Fix permalinks for review.

maybe fix lint before you push? otw lgtm.

May 19 2021, 12:48 AM

May 17 2021

sidkmenon retitled D7869: [dagit] Rename auth header (1/n) from [dagit] Rename auth header (1/3) to [dagit] Rename auth header (1/n).
May 17 2021, 3:27 PM

May 11 2021

sidkmenon updated the diff for D7869: [dagit] Rename auth header (1/n).

Renaming to Dagster-Session-Token

May 11 2021, 9:48 PM
sidkmenon requested review of D7869: [dagit] Rename auth header (1/n).
May 11 2021, 7:53 PM
sidkmenon added inline comments to D7847: [dagster-graphql][client] Fixing `reload_repository_location` and `submit_pipeline_execution`.
May 11 2021, 4:17 PM
sidkmenon added inline comments to D7847: [dagster-graphql][client] Fixing `reload_repository_location` and `submit_pipeline_execution`.
May 11 2021, 4:17 PM
sidkmenon updated the diff for D7847: [dagster-graphql][client] Fixing `reload_repository_location` and `submit_pipeline_execution`.

rebasing & addressing comments

May 11 2021, 4:16 PM

May 10 2021

sidkmenon requested review of D7847: [dagster-graphql][client] Fixing `reload_repository_location` and `submit_pipeline_execution`.
May 10 2021, 10:01 PM

May 7 2021

sidkmenon accepted D7817: [dagit] Fix lint warning.

Oops, sorry for missing this! thanks for the fix.

May 7 2021, 10:00 PM
sidkmenon updated the diff for D7810: [dagit] Add optional authorization header to HTTP requests.

Adding comment and renaming to headerAuthToken

May 7 2021, 9:28 PM
sidkmenon added a comment to D7810: [dagit] Add optional authorization header to HTTP requests.

Hmm I think this works for now because we're planning on using the authToken primarily to attribute runs, etc. to different users, but I think if we really wanted security we'd need to add auth to the websockets interface as well (or stop using it), right? I wonder if we should add a small comment in here explaining that there are still un-authorized GraphQL requests being made from the app in the few places we need websockets?

May 7 2021, 9:23 PM
sidkmenon published D7810: [dagit] Add optional authorization header to HTTP requests for review.
May 7 2021, 8:11 PM
sidkmenon updated the diff for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.

adding comment

May 7 2021, 4:24 PM

May 6 2021

sidkmenon added a reviewer for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP: dgibson.
May 6 2021, 6:04 PM
sidkmenon updated the test plan for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.
May 6 2021, 5:51 PM
sidkmenon resigned from D6468: Renames dev mode to default mode..
May 6 2021, 5:49 PM
sidkmenon updated the diff for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.

Removing React.useMemo on rootServerURI as per @dish's suggestion

May 6 2021, 5:42 PM
sidkmenon added a comment to D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.

The removal of the GEventExecutor

I am pretty sure this is fine to do - and is effectively replicating the way the all-websocket approach worked - but we should add a test cases section specifically about making sure htttp requests can go through while the active run event log subscription is firing data back. You can do this by navigating around while another tab is actively watching a run of many_events from the toyes repo or something.

May 6 2021, 3:52 PM
sidkmenon updated the diff for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.

No longer overrwriting runs page on network errors

May 6 2021, 3:49 PM
sidkmenon added a comment to D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.

This is behavior that is due to the difference between websockets & http - the websocket response does not have an 'error' field on the JS side so the components render differently. Not sure if throwing away the error in this case (to conform to the old behavior) makes sense though -- feels like errors are important to pay attention to!

Hmm - in the screenshot shown we show "GraphQLError - see console for details" but the issue is just that there is no connectivity right? I personally think thats a rough regression. Replacing the contents of the page a big error warning when a polling-refresh query fails due to network connectivity seems bad to me.

Im not sure the exact fix - but I do think we should distinguish "the server sent back an error because something bad happened" from "you couldn't reach the server"

May 6 2021, 3:47 PM
sidkmenon updated the test plan for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.
May 6 2021, 6:41 AM
sidkmenon updated the test plan for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.
May 6 2021, 6:41 AM
sidkmenon updated the test plan for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.
May 6 2021, 6:40 AM
sidkmenon updated the test plan for D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP.
May 6 2021, 6:39 AM
sidkmenon published D7688: [dagit] Make Non-Subscription GraphQL ops use HTTP for review.
May 6 2021, 6:39 AM

Apr 30 2021

sidkmenon added a reviewer for D7687: CHANGES.md typo fix: rexledesma.
Apr 30 2021, 8:48 PM
sidkmenon requested review of D7687: CHANGES.md typo fix.
Apr 30 2021, 8:29 PM

Apr 29 2021

sidkmenon updated the diff for D7675: Changelog for 0.11.7.

Removing automation change

Apr 29 2021, 11:38 PM
sidkmenon updated the diff for D7675: Changelog for 0.11.7.

Updating changelog

Apr 29 2021, 11:37 PM

Apr 27 2021

sidkmenon requested review of D7623: [docs][easy] typo fixes on docs site banners.
Apr 27 2021, 6:04 PM

Apr 26 2021

sidkmenon updated the summary of D7602: [dagster-postgres] [easy] Quick fix in listen/notify logic.
Apr 26 2021, 8:28 PM
sidkmenon requested review of D7602: [dagster-postgres] [easy] Quick fix in listen/notify logic.
Apr 26 2021, 8:27 PM
sidkmenon updated the diff for D7504: [docs] Dagster GraphQL Python Client [4/4].

Reformatting reload_repository_location docs

Apr 26 2021, 8:01 PM
sidkmenon added inline comments to D7504: [docs] Dagster GraphQL Python Client [4/4].
Apr 26 2021, 5:28 PM