I definitely think this is better than trying to re-derive the pipeline from the function args
feels like were jamming two substantially different things in to one function to share a relatively small amount of code/complexity - I bet with some light refactoring you could have a separate log_ function for this metadata and things will feel much better
theres a lot being swallowed by this broad except - I agree that telemetry should fail silently for transient error - but if something was wrong in the instanceof checks above for example we would never find it
does this need to be this flexible given the current 2 callsites? Part of the reason i argued for supplying the pipeline directly is we could be really clear about the expected type (and use check.inst_param for example)
could add an interface if we wanted for reconstructable pipeline
we should get the instance first and thread it down log_repo_starts. Don't add additional calls to DagsterInstance.get()
minor nit: i can be nice to do early returns to avoid excessive nesting:
if not os.path.exists(telemetry_id_path): return None with open(telemetry_id_path, 'r') as telemetry_id_file: telemetry_id_yaml = yaml.safe_load(telemetry_id_file) if not isinstance(telemetry_id_yaml.get(INSTANCE_ID_STR), six.string_types): return None return telemetry_id_yaml[INSTANCE_ID_STR]
as noted above, thread an instance down to here
@alangenfeld what should this look like in a completely migrated world? this is pretty rough
I'd consider making this a namedtuple with checked argument, if nothing that just for documentation
|98 ↗||(On Diff #14276)|
consider just IReconstructablePipeline I prefix for "interface"
based on the two existing callsites - the check above should change to ReconstructableExecutablePipeline and then this if else block goes away
put some explanation here about why we want to flatten to a single schema type from the different actions.
maybe a name like TelemetryEntry, TelemetryRow, Telemetry___ would be good too
maybe slap some weird sigils in here like '<<unable_to_write_instance_id>>'
good stuff thanks for knocking this out
This is interesting. This is for a later diff but I wonder if we need to start having the requirement that wrapped functions have an instance argument so that we make sure we get the correct one...