Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modules #274

Merged
merged 122 commits into from
Dec 2, 2021
Merged

Modules #274

merged 122 commits into from
Dec 2, 2021

Conversation

lorenzleutgeb
Copy link
Member

@lorenzleutgeb lorenzleutgeb commented Oct 27, 2020

Codebase Modularization and API Streamlining

The idea of this PR is to make development of Alpha as well as applications using Alpha less cumbersome by partitioning the existing code into smaller, easier to understand packages, as well as providing a better public API for applications using Alpha.

Modules

The codebase is divided into the following modules: alpha-api, alpha-commons, alpha-core, alpha-solver and alpha-cli-app.

Module alpha-api

The api module consists mainly of interfaces specifying the objects API consumers (i.e. applications using Alpha) interact with, such as programs, rules, atoms, literals and answer sets. It also holds implementations of some configuration classes that are used across modules.

The rationale for specifying all API objects through interfaces and keeping these in a separate module is that it should be possible for implementations to be switched out or changed without affecting applications using the API.

Module alpha-commons

The commons module holds implementations of the "basic building blocks" of ASP programs, such as terms, atoms, literals, etc. which are used across all other modules. These implementations can be instantiated through factory methods (such as Atoms.newBasicAtom(...)), but are not visible to other modules themselves in order to avoid accidental dependencies on implementation-specifc behavior in code using those implementations.

Module alpha-solver

This module provides an implementation of the Alpha interface which is the main API entry point for applications. It is separate from commons and core to avoid API clients having compile-time dependencies against types from alpha-core. alpha-core is an "implementation" scoped, i.e. "internal" dependency of alpha-solver, which means that it is a runtime- but not a compile-time-dependency of code depending on alpha-solver.

Module alpha-core

alpha-core implements Alpha's core functionality, i.e. program parsing, program transformations, grounding and solving. It depends on alpha-commons and alpha-api. While the core module is by far the largest, it is not intended for applications to directly depend on alpha-core, but rather use it through the public API realized in the api, commons, and solver packages.

Module alpha-cli-app

The cli-app module uses the public API to provide Alpha's functionality in a commandline application. It holds a Main class, commandline argument parsing and various utilities for use of Alpha as a commandline-application.

@lorenzleutgeb
Copy link
Member Author

lorenzleutgeb commented Dec 14, 2020

Hey @madmike200590! I am happy to see that you decided to jump in and continue. To be honest I anticipated the split to go much smoother. Turns out that I forgot that we need to split the API from implementations (which causes quite a number of changes), and there are some rough edges like the "Standard Library".

@madmike200590
Copy link
Collaborator

Hey @madmike200590! I am happy to see that you decided to jump in and continue. To be honest I anticipated the split to go much smoother.

Thank you for getting this off the ground! I've wanted to do something in this branch for some time now, but was tied up with PR #268.

Turns out that I forgot that we need to split the API from implementations (which causes quite a number of changes), and there are some rough edges like the "Standard Library".

Yeah, I also only fully realized that when looking at your changes. Still not sure about the nicest way to go, but here are my thoughts so far:

  • We definitely want a split between "core API" (what the solver uses) and public API, because we don't want methods like FixedInterpretationLiteral#getSatisfyingSubstitutions in public API types.
  • On the other hand, we have to make sure that everything used in the solver itself conforms to the "core API", ideally that can be statically guaranteed without weird instanceof constructs that only blow up at runtime.

So what I'm trying to do now is to create a separate hierarchy of "Core" types, i.e. CoreLiteral, CoreAtom, etc. that do not extend the "public API" types, but are created from them using mapping methods, e.g. CoreLiteral#fromLiteral or something like that. While that undoubtedly adds some overhead and very similar code in a few places, it's the neatest way I see to fully decouple the public API from internal workings of our backend and vice-versa. I expect to have a first working draft of that by end of this week, let's see how it looks when it's actually there and doing something...

@lorenzleutgeb
Copy link
Member Author

lorenzleutgeb commented Dec 14, 2020

I see. Aggregates are definitely very useful as well!

And yes, I also ran into the same troubles (public API and internal/core API). I wanted to resolve it with generics, but this is very tedious so I gave up. Your approach with mapping the public types to core types is very well doable! Give it a spin!

I can't wait to add Alpha (just the solver, not the CLI) as a Gradle dependency in some application project for the first time 😆

@madmike200590 madmike200590 linked an issue Dec 18, 2020 that may be closed by this pull request
3 tasks
@madmike200590
Copy link
Collaborator

madmike200590 commented Dec 18, 2020

@lorenzleutgeb, I have a gradle-noob-question that formed in my head while fiddling apart public and "core-internal" APIs...
Ultimately, I think we want to ensure that the core module is only ever a runtime dependency for any application using Alpha through the API defined in api. Especially, I think we want to ensure that it is not (easily through a "normal build workflow") possible for an API consumer to directly compile against types from core (e.g. InternalProgram etc).

In order to achieve that, we need to have all relevant functionality exposed through interfaces in the api module.
However, we also need users to be able to instantiate implementations. Considering the following code snippet from a hypothetical application api-using-app:

class App {
    public static void main(String[] args){
        Alpha alpha = new AlphaImpl();
        alpha.solve(....);
        ....
    }
}

Since AlphaImpl resides in core, our hypothetical API consumer would need a gradle config looking something like

dependencies {
	implementation group: 'at.ac.tuwien.kr',     name: 'alpha-api',      version: '1.0.0'
	implementation group: 'at.ac.tuwien.kr',     name: 'alpha-core',    version: '1.0.0'
}

In my opinion, this would be a problem, since it would make expose the types meant for internal use from alpha-core while the only type that should be exposed is AlphaImpl.

The only way around this that I currently see is to have an additional module, let's call it alpha-solver that basically just contains AlphaImpl and has the following dependencies:

dependencies {
	api group: 'at.ac.tuwien.kr',            name: 'alpha-api',      version: '1.0.0'
	implementation group: 'at.ac.tuwien.kr',    name: 'alpha-core',    version: '1.0.0'
}

As far as I understand gradle dependency scopes, that would mean applications depending on alpha-solver can (as intended) compile against alpha-api as well as AlphaImpl, but get alpha-core (against which alpha-solver is compiled) only as runtime dependency, thereby ensuring they cannot accidentally use types from alpha-core in their application code.

Since I'm much more confident with maven than gradle,

  • Is this the right way to go about this in gradle?
  • Can you think of an alternative that achieves the same without the additional module? I can't think of anything simpler, but still feels like a bit much overhead...

@lorenzleutgeb
Copy link
Member Author

Hmm, I am not sure whether Gradle is the correct layer to address this. I guess it should be JPMS which allows us to specify which packages of our module should be visible to the outside. And then being careful about marking classes pubic.
I think adding another module or adding JPMS config can also be done rather independently once modularization is done.

Copy link
Collaborator

@AntoniusW AntoniusW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this as intermediate review: some comments need addressing. Currently at 307/436 reviewed files and progressing. We are getting there. :-)

@AntoniusW
Copy link
Collaborator

Okay, I am done with the reviewing, all files viewed, finally. There are some of my comments in the last week that should be addressed and there is a git conflict with the build.gradle. But besides that, this PR is ready for merging! Very good work, thank you @lorenzleutgeb and @madmike200590 !

@lorenzleutgeb
Copy link
Member Author

lorenzleutgeb commented Nov 29, 2021

Regarding the conflicting file build.gradle. The conflict is as follows:

CONFLICT (modify/delete): build.gradle deleted in modules and modified in master.
                          Version master of build.gradle left in tree.

This is just because we have changes to build.gradle on master, and deleted it in modules. Funnily enough, GitHub opines that "These conflicts are too complex to resolve in the web editor".

I merged master into modules ("control merge") to resolve, resulting in c65b8ea.

@madmike200590 do you have time to address the unresolved conversations this week? Otherwise just ping me and I'll have a go.

Copy link
Collaborator

@AntoniusW AntoniusW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have a response for my old comment in alpha-cli-app/build.gradle.kts but other than that, everything is fine!

@lorenzleutgeb
Copy link
Member Author

Looks like we're good to merge, yay!

@madmike200590 do you want to rebase this branch, to clean up the history? There are commits messages starting with "WIP" or "Intermediate commit". I'd like to avoid such commits on master. At the same time, I'd also like to avoid squashing this rather large branch into one single commit.

@madmike200590
Copy link
Collaborator

Looks like we're good to merge, yay!

@madmike200590 do you want to rebase this branch, to clean up the history? There are commits messages starting with "WIP" or "Intermediate commit". I'd like to avoid such commits on master. At the same time, I'd also like to avoid squashing this rather large branch into one single commit.

Great! While I see your point about the WIP commits, I don't really want to do a rebase only for that - especially since it's not like it'd be useful to try checking out "halfway done" versions of the modularization. If that's ok with everyone, I'd just go ahead and squash everything.

@lorenzleutgeb lorenzleutgeb merged commit 21d074a into master Dec 2, 2021
@lorenzleutgeb
Copy link
Member Author

I took the liberty to squash and merge, cutting down the merge commit message a lot. I used Co-authored-by (also used in the Linux kernel) to indicate that we share authorship of the commit. I'm really happy that we made it :)

@lorenzleutgeb lorenzleutgeb deleted the modules branch December 2, 2021 15:01
@madmike200590
Copy link
Collaborator

Great, thanks! It's a huge step forward and after all the time this took, it's great to have this finally "live" :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants