Refactor the data class in teal

[donyunardi]

Summary

We're improving the teal's data class (tdata, TealData, etc) and how it's being used in teal framework.
Read the milestone description here

1. Introduction of teal_data

Target: done by first week of sprint 1 (1 week)

• [summary] data refactor teal#895
  • class definition introduce teal_data class teal.data#178
  • introduced a new constructor of FilteredData Introduce teal_data class teal.slice#470
  • handle of a new class in teal Introduce teal_data class teal#925
  • fixed example apps Make JoinKeys related changes due to refactor teal.gallery#92

2. Task to do after the foundation code is merged:

Target: done by end of sprint 1

• [Code Parser]: Ability to figure out object dependencies based on a static code teal.code#133
• Simplify data constructor for join_keys teal.data#162
• Write a new vignette about teal_data teal.data#174
• change new_teal_data arguments teal.data#169
• Introduce within.qenv teal.code#127
• [Feature Request]: Change JoinKeys to something simpler teal.data#78

CHECKPOINT

sprint 1

3. Introduction of ddl

Target: done by the end of sprint 2

• class definition: Introduce the new DDL teal#957
• [Feature request] Introduce validate method for teal_data teal.data#182
• Replace TealData with teal_data in the docs teal.modules.general#591
• Replace TealData with teal_data in the docs  teal.modules.clinical#842
• Replace TealData with teal_data in the docs teal.osprey#231
• Replace TealData with teal_data in the docs teal.goshawk#243
• Replace TealData with teal_data in the docs teal.modules.hermes#339
• https://github.com/insightsengineering/teal.modules.helios/issues/102
• implementation of internal connectors Introduce the new DDL - connectors #38
• Release teal internally after we merge DDL PR.

4. Introducing teal_data to the modules

Target: start this part by in sprint 3

Replacing data argument from tdata to teal_data will introduce breaking change. Problems in teal.transform can be mitigated by making "patches", this will give us possibility to release teal even if teal.transform isn't a final version.

• Hard deprecate (and remove) old teal.data classes and clean documentation to contain only teal_data.
• Don't allow to specify initial env and a code teal.code#145
• Use same data object for teal_data and in modules teal#937
  • Fix datasets arg in data_extract_srv, merge_expression_srv to work with teal_data teal.transform#159
  • Fix modules to use teal_data instead of tdata teal.modules.general#588
  • Fix modules to use teal_data instead of tdata teal.modules.clinical#838
  • Fix modules to use teal_data instead of tdata teal.modules.hermes#338
  • Fix modules to use teal_data instead of tdata teal.osprey#230
  • Fix modules to use teal_data instead of tdata teal.goshawk#242
  • https://github.com/insightsengineering/teal.modules.helios/issues/101
• Replace TealData with teal_data in the vignettes + Using teal_data_module teal#960
• Write a new vignette about teal_data teal.data#174
• adjust technical blueprint to teal_data refactor teal#984
• join_keys vignette teal.data#202
• [Feature Request]: Set default parent teal.data#203
• https://github.com/insightsengineering/coredev-tasks/issues/488

Q: Instruction on migration, should we write vignette (static) or other platform, i.e. GitHub Discussion?
A: We will use GitHub Discussion insightsengineering/teal#945

Q: We have functions (or PR) to support upgrade and downgrade of teal_data. Does this mean we need to keep old classes (i.e. TealData)? How long do we want to support these functions?
A: We're not supporting the old ways. As discussed and agreed upon, the next release will be breaking change.

5. Release teal packages to CRAN

Target: start releasing in the beginning of Jan 2024

• teal.code
• teal.data
• teal.slice
• teal.transform
• teal
• teal.modules.general
• Rest of the teal modules released to GH (potentially even to CRAN?)
• connectors internally

Breaking changes should not occur once sent on CRAN.

UPDATE: Release of teal frameworks (not module), will be done here: #42

Related issues for later

• Remove CodeClass from FilteredData teal.slice#471
• Remove access to data from UI modules teal.slice#132
• [Bug]: Concept of delayed creates indirect dependency between data and UI elements teal.transform#111
• Remove insertUI in the teal::init teal#669
• [Feature Request]: Make a data module teal#860
• [Feature Request]: Default Dataset class for other input data type  teal.slice#483

Addressing step 1-4 automatically closes:

• Dataset$recreate calling Dataset$initialize is confusing teal.data#26
• Add section server value teal.data#39
• Add print function to TealDataConnection objects teal.data#12
• TealDataConnector incorrect behaviour teal.data#72
• Reproducible metadata attribute teal.data#76
• Update dataset_connector documentation teal.data#94
• Create set_ui for TealData teal.data#109
• Expose TealDataset objects out of teal.data teal.data#106
• Allow to create an empty Dataset object teal.data#10
• Refactor DataAbstract to use the new methods for joining and splitting keys teal.data#14
• get_code of DatasetConnector object does not include codes that generated Dataset objects teal.data#28

Definition of Done

• Completion of the refactors where we introduce new teal_data and ddl class with the implementation to teal and teal modules packages by end of Dec 2023.

[donyunardi]

@insightsengineering/nest-core-dev

Release teal.data internally once we introduce teal_data: insightsengineering/teal.data#171
We need to do this before CRAN release.

We need to rethink this.

teal.data is a blocker for other teal framework packages so teal.data has to go to CRAN first.
If we release to CRAN after the refactor, that would take too long.

I thought of couple options:

Option 1

1. Provide soft-deprecate message on related functions.
2. Release to CRAN and internal
3. NOT merge Introduce teal_data class teal.data#171 to main but instead to refactor branch.

Option 2

1. Merge Introduce teal_data class teal.data#171 to main.
2. Provide soft-deprecate message on related functions.
3. Release to CRAN and internal

Option 3 (what's currently suggested)

1. Merge Introduce teal_data class teal.data#171 to main.
2. Provide soft-deprecate message on related functions.
3. Release to internal only
4. Finish the refactor branch
5. Remove all soft-deprecated functions
6. Release to CRAN and internal

I think option 3 is quite risky and could affect our CRAN timeline.
I think option 1 would be ideal.

Please let me know that you think.

[m7pr]

I think it would be really awkward to upload teal.data to CRAN as a first official release and have deprecation messages already. If we do not think 3 is feasible in the timeline we hoped it go get done, then I would suggest

1. uploading current main to CRAN without the soft-deprecation messages,
2. uploading another version to CRAN with soft-deprecation messages once the refactor is merged,
3. uploading another release after another 2 months with a deletion of deprecated functions, so that users have a time to realize some part of functionality is being removed

[kartikeyakirar]

I agree with Marcin on the initial deprecation message release, also believe in providing users ample time to adapt to the changes for a smoother transition.Option 1 with approach is to first upload the current main version to CRAN without the soft-deprecation messages, and then follow up with another version containing the deprecated messages, which I believe would be an improved approach.

[vedhav]

Now for some unpopular opinion.
The two problems that I see are:

1. Not being able to release old + new compatible release on GitHub followed by the new only release on CRAN because creating deprecation will take time.
2. Releasing CRAN with deprecated functions/methods

What if?
We can take a release branch and just merge the new changes without compatibility and release on CRAN and come back to do the old + new compatibility release on the main branch on GitHub followed by quickly updating the GitHub to the CRAN version.
Not sure if this is feasible, but if it's possible my vote will be for this. Otherwise, I'm good with Marcin's proposal.

P.S While typing this I see how insane this scenario is

[m7pr]

I don't understand this proposition

[vedhav]

Nevermind, I was just thinking out loud. We scraped my proposition in the daily :)

[donyunardi]

@m7pr makes the most sense to me.
Let's discuss and make decision next week when the whole gang is in.

[lcd2yyz]

Although I perfectly understand the struggle of "don't want to show my code to the world unless it's the best version", the "cost" seems too high to justify Option 3.

@m7pr's proposal looks optimal to me. This can also help to largely de-risk our upcoming PI, or any foreseen/unforeseen impact from the major refactor. It will allow us to keep the target of releasing the whole teal framework by end of year.

[m7pr]

We still have this week + next 2 weeks before the next increment starts. Maybe we can merge refactor within 3 weeks, and then we can release the final version that is ready and polished? I also think the refactor was non-committed goal/milestone for this increment so I dont think we should struggle to release this week if we have major conerns

[chlebowa]

My opinion is probably deeply ignorant but I don't think adjusting refactor pace to release schedule is the right way to go. I think it's better to put some effort in the refactor (which I firmly believe we can finish by the end of the year) than release prematurely. Having deprecation warnings in the first (CRAN) release may be a tad confusing and may undermine user confidence in the product as a whole.

[m7pr]

If there refactor finishes at the end of the year, then I think it's fine to release current main (without deprecation messages) so that we can unblock the process of releasing other packages in the line.

[gogonzo]

I suggest to go with initial plan:

1. Release on GitHub with soft deprecation
2. Immediately after GH release we remove deprecated stuff on main and release on CRAN.

Thanks to above teal.data won't be postponed.

• uploading current main to CRAN without the soft-deprecation messages,
• uploading another version to CRAN with soft-deprecation messages once the refactor is merged,
• uploading another release after another 2 months with a deletion of deprecated functions, so that users have a time to
• realize some part of functionality is being removed

Releasing long awaited package to CRAN and making API changes soon after is not a good idea. We will have to support users using old version and new version

[m7pr]

Thanks @gogonzo for the message. I just dont understand what you want to

1. Release on GitHub? The current main? Or what? Or the refactor once it's merged?
2. By deprecated stuff you mean messages, or the whole functionality?

Releasing long awaited package to CRAN and making API changes soon after is not a good idea. We will have to support users using old version and new version

Totally, but we will support this only by some small period of time.

[gogonzo]

Totally, but we will support this only by some small period of time.

I bet someone from NEST will announce "teal.data is on CRAN" and we will have a problem.

[m7pr]

got it. agree that we should release the final product to prevent supporting both versions and prevent people becoming discouraged becuase they need to learn new version, once they invested the time and the energy to learning the first version which is not finished

[donyunardi]

We re-discussed our strategy again and updated the original comment with the new agreed release plan and timeline.
The main changes is to not perform any internal release and force breaking change for modules.

[donyunardi]

Refactor is completed.
Closing.