klio icon indicating copy to clipboard operation
klio copied to clipboard

[core][WIP] Rename type to type name

Open shireen-bean opened this issue 4 years ago • 1 comments

Description

Currently, inputs and outputs can be given names in klio-job.yaml. If no name is provided then one is generated based on the type of I/O, something like "gcs1".

Right now this name is basically dropped when the KlioConfig object is created. It is removed as part of config pre-processing and klio_config.events.inputs stores each I/O object in a list (so I/O cannot be accessed by name), and if as_dict() is used, "name" will not be included.

There is currently a "name" attribute on the base IOConfig class, however this is a class-level attribute that refers to the string name of the IO config type used in klio-job.yaml, e.g. "gcs" or "pubsub", and so would conflict with actually trying to add a "name" attribute to store the name of the individual instance itself.

This PR disambiguates "type" from "name" to "type_name", adds "name" into the config dict and drops the name generation.

Testing

  • Updated unit tests
  • Tested in test jobs streaming and batch

Checklist for PR author(s)

  • [x] Format the pull request title like [cli] Fixes bugs in 'klio job fake-cmd'.
  • [x] Changes are covered by unit tests (no major decrease in code coverage %) and/or integration tests.
  • [x] Document any relevant additions/changes in the appropriate spot in docs/src.
  • [x] For any change that affects users, update the package's changelog in docs/src/reference/<package>/changelog.rst.

shireen-bean avatar Mar 30 '21 16:03 shireen-bean

So I think there's one main thing not yet supported in this PR: it should be possible to get a config by name, eg klio_config.data.inputs["my-data-input"].

For some reason I didn't include name as an attr, I can't remember why, but looking again I don't think that was the right approach, so you'll have to adapt my code to account for that (or do it your own way if you have a better idea 😄 ).

Yeah I have this on another branch locally and went down pretty deep into that exploration 😓 i THINK the reason you didnt use attr is cuz to maintain backwards compatibility between klio_config.data.inputs["my-data-input"] and klio_config.data.inputs[0]we can make a dictionaryand have the keys be mixed strings and ints but I hit an issue where attr is enforcing string type making it into klio_config.data.inputs["0"] 🤦🏽 . So ^^ this current PR exposes it like (still a list) [{name: "thing1"}, {"name":"thing2"}] so you could at least access the name if you wanted to and the change would still enable muti-named-inputs. Might also comes down to if we will keep enforcing a list vs a dictionary.

shireen-bean avatar Mar 30 '21 18:03 shireen-bean