PathmindAI / pathmind-webapp

The Pathmind Webapp
https://dev.devpathmind.com
Apache License 2.0
1 stars 0 forks source link

Let users rename the RL experiment #2501

Closed slinlee closed 3 years ago

slinlee commented 3 years ago

Right now we require that the RL experiment is named RLExperiment

  1. this is causing a lot of errors because the default name isn't always the same
  2. users are in the habit of renaming this experiment
kepricon commented 3 years ago

In short, we need to have expected names that are even case sensitive for Main agent, Pathmind Helper, Simulation, RL Experiment under the current implementation.

exports.zip Here are my test models based on SimpleStochastic that have 8 cases. export1: normal case(Main, pathmindHelper, Simulation) export2: Simulation->Simulation1 export3: pathmindHelper -> pm export4: pathmindHelper -> pathmindHelper1 export5: pathmindHelper -> PathmindHelper export6: Main -> Main2 export7: Main -> test export8: it has two pm helpers

only export1 is working, other models are NOT working. cc/ @slinlee @ejunprung

saudet commented 3 years ago

It should be possible to use a singleton similar to PathmindHelperRegistry: https://github.com/SkymindIO/nativerl/blob/dev/PathmindPolicyHelper/PathmindPolicyHelper.alp#L1231 This way we just need to call a static method like RLExperiment.getInstance() so it doesn't matter what the name is.

kepricon commented 3 years ago

the current model upload process consists of two parts: model validation from Webapp and model analysis from MA. 1) model validation from Webapp

2) model analysis from MA

To make it enable to rename RL experiment and others(SImulation, Main, PathmindHelper), we need to use Java reflection or byte code analysis library(we are using https://asm.ow2.io/ in webapp to check pm helper now) since Model analyzer is relying on the exact name of Simulation, Main, PathmindHelper, RLExperiment. (for example, https://github.com/SkymindIO/nativerl/blob/dev/nativerl/examples/train.sh#L2)

My idea is

  1. extract the names of Simulation, Main, RLExperiment, PathmindHelper from https://github.com/SkymindIO/pathmind-webapp/blob/dev/pathmind-services/src/main/java/io/skymind/pathmind/services/project/ByteCodeAnalyzer.java
  2. pass the names to Model Analyzer and get analyzed
  3. export the names in training script(script.sh) to pass it to natvierl
  4. use the names for training.

we can support user-defined names that can help a lot of our customers and internal users.

any thoughts @slinlee @ejunprung @alexamakarov ?

I think Alex is also working on refactoring Model Upload process from https://github.com/SkymindIO/pathmind-webapp/pull/2530 afaik, Alex is moving all validation work to MA. I hope this comment to avoid duplicated work or minimize refactoring work from my work.

slinlee commented 3 years ago

That sounds like a good plan, @kepricon .

If this is a good time to consolidate to just the model analyzer, that's good.

ejunprung commented 3 years ago

How do we know which is the correct Simulation and Main if a user has more than one? RLExperiment and PathmindHelper is limited to one but Simulation and Main can have many.

kepricon commented 3 years ago

good question, looks like I can get root agent from Experiment(Simulation and RLExperiment).

  1. If there are Simulation and RLExperiment -> Simulation has a higher priority.
  2. If there are more than two Simulations -> the simulation that has "Simulation" name has a higher priority.
  3. If there is only one Simulation or RLExperiment, -> we will use the experiment

and I will consider the agent that is from the selected experiment class.

ejunprung commented 3 years ago

Gotcha, that sounds good!

alethander commented 3 years ago

I hope this comment to avoid duplicated work or minimize refactoring work from my work.

Thanks, @kepricon , for the detailed explanation. From the current changes on both PR there is no duplicating work, but there is already some upcoming minor conflict with refactoring I have done so far. Nothing critical, I will be able to merge this one with a reworked model upload.