Feature/remove functionality level #649

Grifs · 2024-02-13T12:31:15Z

Describe your changes

Remove the functionality layer from the config and move all fields to the top layer.

Related issue(s)

Closes #500

Type of Change

Breaking change (fix or feature that would cause existing functionality to not work as expected)
New functionality (non-breaking change which adds functionality)
Major change (non-breaking change which modifies existing functionality)
Minor change (non-breaking change which does not modify existing functionality)
Bug fix (non-breaking change which fixes an issue)
This change requires a documentation update

Checklist

Requirements:

I have read the CONTRIBUTING doc.
I have performed a self-review of my code by checking the "Changed Files" tab.
My code follows the code style of this project.

Tests:

I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.

Documentation:

Proposed changes are described in the CHANGELOG.md.
I have updated the documentation accordingly.

Test Environment

codecov · 2024-02-13T13:33:43Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (9d9e5a7) 92.51% compared to head (dbb1237) 92.46%.

Files	Patch %	Lines
src/main/scala/io/viash/config/package.scala	93.75%	4 Missing ⚠️
src/main/scala/io/viash/ViashTest.scala	80.00%	2 Missing ⚠️
src/main/scala/io/viash/config/Config.scala	96.42%	2 Missing ⚠️
src/main/scala/io/viash/Main.scala	75.00%	1 Missing ⚠️
...c/main/scala/io/viash/runners/NextflowRunner.scala	92.85%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #649      +/-   ##
===========================================
- Coverage    92.51%   92.46%   -0.06%     
===========================================
  Files          129      127       -2     
  Lines         3889     3889              
  Branches       635      623      -12     
===========================================
- Hits          3598     3596       -2     
- Misses         291      293       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…rmat

Grifs · 2024-02-13T14:53:54Z

The built script currently still has the meta_functionality_name variable set to refer to the component's name. Should we add a second meta_component_name together with the matching VIASH_META_COMPONENT_NAME name?

In time the meta_functionality_name could be removed if/when so wanted.

Additionally, ns exec has a functionality-name identifier. Should we also duplicate this with component-name?

rcannood · 2024-02-14T08:56:26Z

Great stuff! Before I review the PR (phew), some questions:

Is the change still backwards compatible for now?
Do we test whether this backwards compatibility still works?
The built script currently still has the meta_functionality_name variable set to refer to the component's name. Should we add a second meta_component_name together with the matching VIASH_META_COMPONENT_NAME name?

Yes! Or maybe simply meta_name and VIASH_META_NAME?
In time the meta_functionality_name could be removed if/when so wanted.

Yes but let's not rush this
Additionally, ns exec has a functionality-name identifier. Should we also duplicate this with component-name?

Or simply name?

Grifs · 2024-02-14T09:11:20Z

Great stuff! Before I review the PR (phew), some questions:

Is the change still backwards compatible for now?

Yes, (limited) tests so far "just work". Functionality (no pun intended) is provided by https://github.com/viash-io/viash/pull/649/files#diff-08d6fbd855a302a8c8a679e2d4e5612616e059a9a14020da75697415e974c04cR223

Do we test whether this backwards compatibility still works?

Not yet explicitly (but I believe there is still an external script that is fetched and that is currently still in the old format). It's on my TODO list. (shouldn't take too much time 🤞 )

The built script currently still has the meta_functionality_name variable set to refer to the component's name. Should we add a second meta_component_name together with the matching VIASH_META_COMPONENT_NAME name?

Yes! Or maybe simply meta_name and VIASH_META_NAME?

👍

In time the meta_functionality_name could be removed if/when so wanted.

Yes but let's not rush this

👍

Additionally, ns exec has a functionality-name identifier. Should we also duplicate this with component-name?

Or simply name?

👍

Tweak wording of the deprecation message as not being removed but deprecated

src/main/resources/io/viash/helpers/bashutils/ViashDockerFuns.sh

rcannood · 2024-02-21T13:02:18Z

src/main/scala/io/viash/Main.scala

-          namespace = appliedConfig.config.functionality.namespace,
-          functionalityName = appliedConfig.config.functionality.name
+          namespace = appliedConfig.config.namespace,
+          componentName = appliedConfig.config.name


could be renamed to simply name, or not?

rcannood · 2024-02-21T13:03:44Z

src/main/scala/io/viash/ViashNamespace.scala

@@ -56,13 +56,13 @@ object ViashNamespace extends Logging {
    targetDir: String,
    runnerId: String,
    namespace: Option[String],
-    functionalityName: String
+    componentName: String


Same -- see above!

rcannood · 2024-02-21T13:05:25Z

src/main/scala/io/viash/config/ArgumentGroup.scala


-import io.viash.helpers.data_structures._
-import io.viash.functionality.arguments.Argument
+// import io.viash.helpers.data_structures._


can this be removed?

rcannood · 2024-02-21T13:16:18Z

src/main/scala/io/viash/config/package.scala

+            val buildInfo = conf.apply("info")
+            val conf1 = buildInfo.map{ bi => 
+                conf.remove("info").add("build_info", bi)
+              }.getOrElse(conf)


Why move info to build_info? The config shouldn't contain a info field at this stage yet, right?

...Or are we using this function to read in .config.vsh.yaml files as well? 👀

It is indeed meant to have some sort of compatibility with (old) .config.vsh.yaml files. I wanted to prevent poluting & mixing .functionality.info with the build info under .info. Unlikely to happen often, but it was easy to write too.
Also, the code should be scheduled to be dropped in the future, so any extra complexity here shoud be temporary.

src/main/scala/io/viash/engines/DockerEngine.scala

rcannood · 2024-02-21T13:27:10Z

src/main/scala/io/viash/helpers/DependencyResolver.scala

+        ("functionalityName" -> c.name),
+        ("functionalityNamespace" -> c.namespace.getOrElse(""))


Do these keys need to be renamed?

good catch! I was checking whether you were paying attention! >.>

src/main/scala/io/viash/schemas/CollectedSchemas.scala

src/main/scala/io/viash/wrapper/BashWrapper.scala

Co-authored-by: Robrecht Cannoodt <[email protected]>

Grifs added 5 commits February 12, 2024 18:44

Rework removing functionality

0f1732c

Start fixing testbenches

0212944

move arguments -> argument_groups code, fix more testbenches

0c0f0f0

add json translation for functionality -> config, fix more tests

cc5bb2c

Fix json schema and add changelog entry

5c3f6fd

Reduce namespace test header duplicated definitions of columns and fo…

2167078

…rmat

update documentation of config mods

80efd60

Add name field for ns exec and meta_name in BashWrapper

a7b3797

Grifs force-pushed the feature/remove_functionality_level branch from 773f014 to a7b3797 Compare February 14, 2024 10:07

Add testbench to test a config with a deprecated Functionality

23a1094

Tweak wording of the deprecation message as not being removed but deprecated

Grifs requested a review from rcannood February 14, 2024 11:18

Grifs marked this pull request as ready for review February 14, 2024 11:18

rcannood reviewed Feb 21, 2024

View reviewed changes

Grifs and others added 2 commits February 21, 2024 14:47

Apply suggestions from code review

92f0a16

Co-authored-by: Robrecht Cannoodt <[email protected]>

Additional changes wrt code review

d15c7e2

Grifs requested a review from rcannood February 21, 2024 15:37

rcannood approved these changes Feb 21, 2024

View reviewed changes

Merge branch 'develop' into feature/remove_functionality_level

dbb1237

Grifs merged commit 396dec0 into develop Feb 21, 2024
9 checks passed

rcannood deleted the feature/remove_functionality_level branch April 5, 2024 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/remove functionality level #649

Feature/remove functionality level #649

Grifs commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

Grifs commented Feb 13, 2024 •

edited

Loading

rcannood commented Feb 14, 2024

Grifs commented Feb 14, 2024 •

edited

Loading

rcannood Feb 21, 2024

Grifs Feb 21, 2024

rcannood Feb 21, 2024

rcannood Feb 21, 2024

rcannood Feb 21, 2024

Grifs Feb 21, 2024

rcannood Feb 21, 2024

Grifs Feb 21, 2024

		("functionalityName" -> c.name),
		("functionalityNamespace" -> c.namespace.getOrElse(""))

Feature/remove functionality level #649

Feature/remove functionality level #649

Conversation

Grifs commented Feb 13, 2024 • edited Loading

Describe your changes

Related issue(s)

Type of Change

Checklist

Test Environment

codecov bot commented Feb 13, 2024 • edited Loading

Codecov Report

Grifs commented Feb 13, 2024 • edited Loading

rcannood commented Feb 14, 2024

Grifs commented Feb 14, 2024 • 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

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

Grifs commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

Grifs commented Feb 13, 2024 •

edited

Loading

Grifs commented Feb 14, 2024 •

edited

Loading